Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove 1-D lat/lon variables from mimic reader's available datasets #1392

Merged
merged 13 commits into from Oct 23, 2020

Conversation

joleenf
Copy link
Contributor

@joleenf joleenf commented Oct 6, 2020

Removes the 1-D lat/lon variables from the mimic reader yaml so that lat/lon values are only used as coordinates and do not appear in the available_dataset_ids list.

@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #1392 into master will increase coverage by 0.03%.
The diff coverage is 92.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1392      +/-   ##
==========================================
+ Coverage   90.56%   90.59%   +0.03%     
==========================================
  Files         228      229       +1     
  Lines       33406    34190     +784     
==========================================
+ Hits        30254    30975     +721     
- Misses       3152     3215      +63     
Impacted Files Coverage Δ
satpy/enhancements/mimic.py 0.00% <0.00%> (ø)
satpy/tests/reader_tests/test_mimic_TPW2_lowres.py 97.58% <97.58%> (ø)
satpy/readers/mimic_TPW2_nc.py 80.95% <100.00%> (-9.41%) ⬇️
satpy/tests/reader_tests/test_mimic_TPW2_nc.py 95.94% <100.00%> (-0.06%) ⬇️
satpy/tests/test_composites.py 99.91% <0.00%> (+0.02%) ⬆️
satpy/composites/__init__.py 84.66% <0.00%> (+1.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12917ab...57aa58f. Read the comment docs.

@coveralls
Copy link

coveralls commented Oct 6, 2020

Coverage Status

Coverage decreased (-0.02%) to 90.545% when pulling 57aa58f on joleenf:iss1371 into 12917ab on pytroll:master.

@djhoese djhoese changed the title Iss1371 Remove 1-D lat/lon variables from mimc reader's available datasets Oct 6, 2020
@djhoese djhoese self-assigned this Oct 6, 2020
@djhoese djhoese changed the title Remove 1-D lat/lon variables from mimc reader's available datasets Remove 1-D lat/lon variables from mimic reader's available datasets Oct 6, 2020
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, it was that simple?

I changed the title since we use the PR and issue titles in our release notes and thought this would be more clear to those unfamiliar with the issue number(s).

@joleenf
Copy link
Contributor Author

joleenf commented Oct 6, 2020

Yes. It seems that it was that simple, which makes me nervous that I actually missed something. However, I checked with a high and low resolution file, along with the test data. I should probably create a second test dataset to replicate the lower resolution files now that I think of it.

@joleenf
Copy link
Contributor Author

joleenf commented Oct 7, 2020

How do I put a hold on this request until I can make sure the testing files are working correctly? I thought they were passing, but they just were not running due to a missing package.

@pnuu
Copy link
Member

pnuu commented Oct 7, 2020

I'm not sure if it's visible to you, but there's a Convert to draft link below the reviewers.

@joleenf joleenf marked this pull request as draft October 7, 2020 12:01
@joleenf
Copy link
Contributor Author

joleenf commented Oct 7, 2020

@pnuu Thanks!

… to the endian of the machine and could fail without necessity.
…float values to a timedelta so that the timed grids can be plotted directly along with the other dataset_ids without causing an error all_available_ids() are generally loaded and saved using the Scene object.
…s for the mimic tpwGrids which specifies a colormap for all the fields.
Just fullfilling the don't use rainbow requirement.  Use spectral with reverse=True
but keep rainbow handy because that is currently the colormap used for MIMIC diagnostic
images on the archive.
Checking for a netCDF variable before adding attributes is a poor way of avoiding an error when running the test dataset.
Instead, check for attributes associated with the variable when it is read and update the metadata appropriately.
Rearrange how the units are defined to better replicate the real datasets.  Add check for units.
@pnuu
Copy link
Member

pnuu commented Oct 19, 2020

The Travis build failures are caused by the newest eccodes or python-eccodes package(s) in conda-forge. @mraspaud is inspectin this in Slack and #1408

@pnuu
Copy link
Member

pnuu commented Oct 20, 2020

I restarted the failed Linux build on Travis, the python-eccodes has now been built on Conda and the tests should pass.

@joleenf
Copy link
Contributor Author

joleenf commented Oct 23, 2020

@mraspaud @djhoese This is ready. This pull request addresses #1392 as stated in initial pull request. It also adds enhancements for the additional data fields contained in the lower resolution imagery. In addition, a test is added for the structure of the lower resolution files.

@joleenf joleenf marked this pull request as ready for review October 23, 2020 11:48
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for taking the time to add tests!

@djhoese
Copy link
Member

djhoese commented Oct 23, 2020

I know the colormap stuff was sub-optimal but I think this is a good workaround. Thanks for all the work.

@djhoese djhoese merged commit 446e24e into pytroll:master Oct 23, 2020
@joleenf joleenf deleted the iss1371 branch October 23, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIMIC reader available_dataset_names returns 1d lat/lon fields
5 participants