Closed BENR0 closed 10 months ago
Attention: 9 lines
in your changes are missing coverage. Please review.
Comparison is base (
e82bf47
) 94.06% compared to head (5191248
) 94.23%. Report is 2 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
pyresample/_formatting_html.py | 94.57% | 7 Missing :warning: |
pyresample/area_config.py | 92.30% | 1 Missing :warning: |
pyresample/geometry.py | 83.33% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
You may want to try installing the pre-commit hooks. It looks like you've made the style checkers very angry :wink:
Other than the style issues, what happens if matplotlib and/or cartopy are not installed? These are not hard requirements of pyresample so we should still show something useful if they are not installed.
@djhoese Yeah I already saw it :-D. I think most of the complaints are in the plotting which I need to refactor a little anyway.
Good point about matplotlib/cartopy didn't think about that. But in that case I think it should just fall back to the normal string representation.
I will also Look into the other suggestions you made.
@BENR0 what's the status of this? also, will this affect performance of the notebooks?
Yes I think that should be possible in general. I already did some work in that direction and also towards having a representation for SwathDefinitions
. It's not done yet though. If you want to include in a release before PCW I could try and refactor what I have to a satisfactory level to be included. Otherwise working on this as well as on the html representation for Scenes is on my list for PCW :-).
Oh then lets leave this as a task for you for the PCW with the goal of merging it by the end of the week.
Current status:
Pre-commit is complaining about missing docstrings:
pyresample/static/__init__.py:1:1: D104 Missing docstring in public package
pyresample/static/html/__init__.py:1:1: D104 Missing docstring in public package
pyresample/static/css/__init__.py:1:1: D104 Missing docstring in public package
Waiting for tests here :)
Ok, so I added some tests for the "cartopy not installed" case. If xarray is not installed I now opted to print the numpy arrays, so there was no need for some extra message for the user. This should work as expected now. There are some things to be noted/ todos:
_repr_html_
for the SwathDefinition
has no Area overview because plotting is to expensive currently in too many cases. This is something I will add again once the G-Ring information is added to the SwathDefinition
SwathDefinition
.What's the current state of this PR? Should it wait for the next PCW? Or can I plan to include it in the next minor release (1.28.0)?
@djhoese This can be merged. The overview for the SwathDefinition
is still missing due to the performance problem. But Martin and I talked about it during PCW and agreed that this is good to go as is and that I will add that at a later point when the problem is solved together with some more refactoring (possibly in conjunction with https://github.com/pytroll/satpy/pull/2167 which should also be good to go and https://github.com/pytroll/satpy/pull/2171).
pre-commit wants a docstring on all of the
__init__.py
files in the static directory. Could you add those (3 total)?
Done
@mraspaud @mraspaud I added some more tests. They are pretty rudimentary and just check if the generated html string contains tags specific to the feature being tested.
While working on this I stumbled about the resolution calculation for the SwathDefinition
in the numpy case again There are two points: First (as mentioned earlier in the discussion) this is very basic and probably not very precise and therefore should be refactored (ideas welcome, maybe there is something that can be reused already?). Second in the xarray case it is possible that the representation breaks because currently the assumption is from a Satpy
view were the Latitude and Longitude DataArrays
in SwathDefinitions
get assigned attributes like name
, resolution
and units
. So in the case were a user creates a SwathDefinition
with xarray
lats/lons without those attributes the representation throws an error. These attributes could be made mandatory if SwathDefinitions
are created with DataArrays
which is a pretty hard constraint I guess or the representation could do more checks and in case these attributes are missing fall back to calculating these as in the numpy case which is "inline" with point one needing some more work. Let me know what you think.
Currently the pure dask.array
and numpy
case print outs look the same. I think the reason that if dask.arrays
are input into the SwathDefinition
the printout still shows them as numpy arrays is the following lines https://github.com/pytroll/pyresample/blob/12892b875221846f0fbc7f68775dbdadd8b68584/pyresample/geometry.py#L802-L804. Anyway this is how it looks like (fixed the resolution decimals by rounding, will push it later):
The xarray
case looks like this:
The AreaDefinition
looks like this (the map portion is folded initially):
What does the SSP
mean on the resolution? Single standard parallel?
This looks pretty good to me. In the future we may want to have an optional WKT display and we may want to handle name/ID and description being empty since pyresample 2.0 won't use them by default (it considers them metadata).
SSP = Sub Satellite Point
Yes I thought about the WKT display also. In any case I think there is some room for improvement.
Looks like there are merge conflicts (and somehow appveyor got run again...wtf). Can you fix up the merge conflicts and see how the rest of CI feels about it?
Alright hopefully fixed now.
Sorry for being so late on this. I was a little concerned by how large the coverage loss was. I checked and it looks like the generate_area_def_rst_list
function isn't covered. Would it be possible to add a test for it?
I was afraid one of you would notice and ask for a test :-D. I always find it difficult to write tests for something like this but I will have a look at it and come up with something. I am not sure if I will be able to do that before my holiday next week though (I have a long train ride I will try to use but no guarantees).
Where are we at with this?
@djhoese sorry for the delay. I added a test now. I am only checking if area_repr
is called because actual checking of the output seems kind of difficult especially due to the plots being included as svg string. The line when an area in the file does not have a _repr_html_
attribute does not get checked. I think this is only the case for stacked and dynamic areas which can not be dumped to yaml anyway right? I can't remember the exact cases why this was needed I can just remember some of the areas throwing errors when I tested this way back. Anyway therefore the test is not really complete I guess.
A couple suggestions and questions, but otherwise coveralls is still saying the SwathDefinition branch of the formatting is not covered. That and the PNG output option. I see mention of swath definitions in your test so any idea why those branches aren't being covered?
I think the SwathDefinition
tests just test the html code part and not the overview plot that's why that branch in the plotting function is not covered. I will try and add tests for the plotting function itself or update the test to also cover the SwathDefinition
branch. The PNG output option could be removed I just left it there as a feature but it's not used in the html representation. Should I remove it then or add a test (even though I am not sure how write a check for that yet :-D).
If the PNG stuff isn't used in the final output then I guess I'm ok with it being removed. Although...it might be nice to export an area definition to a PNG. A test wouldn't be too bad as long as you just checked that it was saving and maybe check that it isn't all black. I guess it is up to you what is best for the future. Or maybe @mraspaud has an opinion.
The problem with the testing of the plotting function is that I actually don't save the plot to disc but generate the string representation to include it in the html later on. I guess I could check if savefig
was called which would be somehow equivalent to checking that something gets saved. I have no idea how to check if the output is all black or not.
/
Given that we're basically depending on another program to output the PNG representation I am OK with just knowing that it isn't empty (0 size).
Ah I see you added specific tests that check savefig.
pre-commit.ci autofix
Opened an issue about the private _obj_repr
function here: https://github.com/pydata/xarray/issues/8551
This adds
_repr_html_
to theAreaDefinition
class. In Jupyter notebooks therefore instead of a string a nicely formatted text is show together with a map overview (cartopy plot with borders and coastlines) as can be seen in the example below. The map by default is folded right now to save space (for the screenshot I unfolded it).The way this is setup is heavily influenced by how xarray implements the
_repr_html_
. Basically I used the same directory/ file structure. I also copied some code (the html with the svg icons, the function to read the static data) which I did not attribute yet.While functionally ready some considerations before this get merged:
Apart from some refactoring of the plotting function (removing unecessary code and comments) I was thinking about maybe moving the plotting function into the class?
Only the
AreaDefinition
has a html representation as of now. It would be nice if something similar could be done forSwathDefinition
to make the look and feel more homogeneous.[ ] Tests added
[ ] Tests passed
[ ] Passes
git diff origin/main **/*py | flake8 --diff
[ ] Fully documented