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

Add arbitrary filename suffix to ABI L1B reader #1380

Merged
merged 5 commits into from Dec 8, 2020

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Sep 23, 2020

This addresses a user request in SIFT (ssec/sift#299) where @jgerth has some ABI L1b files that are generated by his own processing and are similar but not exactly the same as the original ABI L1b files that fed the processing. This PR allows him to easily name these files in an identifiable way while also providing some metadata to do that identification.

  • Tests added
  • Passes flake8 satpy
  • Fully documented

@djhoese djhoese added enhancement code enhancements, features, improvements component:readers labels Sep 23, 2020
@djhoese djhoese self-assigned this Sep 23, 2020
@djhoese
Copy link
Member Author

djhoese commented Sep 23, 2020

I'm open to suggestions. Maybe this should be called filename_suffix or similar to the orbital parameters we should have a sub-dictionary called data_source_metadata or filename_info or something like that. I want to keep in mind some of the discussion from @carloshorn on #1349 where we may in the future not be restricted to file-like or filename-like data sources.

@coveralls
Copy link

coveralls commented Sep 23, 2020

Coverage Status

Coverage increased (+0.04%) to 90.569% when pulling c107f77 on djhoese:feature-abi-l1b-fn-suffix into 1612d84 on pytroll:master.

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@1612d84). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1380   +/-   ##
=========================================
  Coverage          ?   90.56%           
=========================================
  Files             ?      228           
  Lines             ?    33421           
  Branches          ?        0           
=========================================
  Hits              ?    30269           
  Misses            ?     3152           
  Partials          ?        0           
Impacted Files Coverage Δ
satpy/readers/abi_l1b.py 96.72% <100.00%> (ø)
satpy/tests/reader_tests/test_abi_l1b.py 100.00% <100.00%> (ø)

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 1612d84...c107f77. Read the comment docs.

@mraspaud
Copy link
Member

Why replace an existing standard filename pattern and not add it at the end of the pattern list as we usually do?
Also to avoid ambiguity, is the suffix starting with eg _ that we could have between the creation Time and the suffix?

@djhoese
Copy link
Member Author

djhoese commented Oct 6, 2020

Hm I could call the filename field "testing_identifier" or something to give it more meaning than "suffix" or "filename_suffix".

@ghost
Copy link

ghost commented Oct 7, 2020

Congratulations 🎉. DeepCode analyzed your code in 2.18 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

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.

That works. A comment only.

@@ -81,8 +81,9 @@ def get_dataset(self, key, info):
# although we could compute these, we'd have to update in calibration
res.attrs.pop('valid_range', None)
# add in information from the filename that may be useful to the user
for attr in ('observation_type', 'scene_abbr', 'scan_mode', 'platform_shortname'):
res.attrs[attr] = self.filename_info[attr]
for attr in ('observation_type', 'scene_abbr', 'scan_mode', 'platform_shortname', 'suffix'):
Copy link
Member

Choose a reason for hiding this comment

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

I know this is not new, but I don't really like having to add to this everytime there is a new item popping up in the file patterns. Could these items to be passed to the user be defined in the yaml file instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't be against it, but I'm not sure what you mean. Do you mean the list of what should be added to the attrs should be in the YAML? Or do you mean that the actual values for these are defined in the YAML?

If the former, would that be on each file type? Or maybe in the main reader section?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the former, in the file type definition, but maybe the reader section is more appropriate

Copy link
Member Author

Choose a reason for hiding this comment

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

@mraspaud I was going to try to implement this, but realized the problem with defining this list of attrs in the reader (the more logical place) makes the code difficult because the file handler's don't have access to the reader kwargs...oh unless I modify the ABI Base file handler and have it expect this kwarg. 🤔 Will look tomorrow, but let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I think having them in the filetype info would be ok, because you might want to have different attributes for different filetypes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but then I have 16 repeat blocks of YAML plus the parameters are typically per "style" of file pattern and not necessarily file type. I'd just like to avoid this "clean up". It is beginning to not be clean at all. Let me know how strongly you care.

Copy link
Member

Choose a reason for hiding this comment

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

I care about how clean the solution is more than where this info is :) so it's up to you!

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this: If this list of attributes changes again then I promise I will find a clean solution for this where the values are put somewhere more configure-y (YAML). Honestly, we could probably just copy all the filename parameters if they aren't already defined earlier in get_dataset. I'm going to merge this.

Copy link
Member

Choose a reason for hiding this comment

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

ok, it's up to you. I'll take you on that promise though :D

Copy link
Member Author

Choose a reason for hiding this comment

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

The abi l1b reader (at least the get_dataset method) is gross enough as it is. It needs to be refactored but this isn't the PR or the time for it. I know that technically breaks the "law of refactoring" but I didn't get enough sleep last night to care 😉

@djhoese djhoese merged commit 9101690 into pytroll:master Dec 8, 2020
@djhoese djhoese deleted the feature-abi-l1b-fn-suffix branch December 8, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants