-
Notifications
You must be signed in to change notification settings - Fork 300
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
Improve predictability of DataQuery, DataID, and dependency tree #3018
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3018 +/- ##
==========================================
- Coverage 96.10% 96.08% -0.02%
==========================================
Files 377 378 +1
Lines 55163 55213 +50
==========================================
+ Hits 53012 53050 +38
- Misses 2151 2163 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 12381834226Details
💛 - Coveralls |
Includes changes to loading compositors to a DataID with all query parameters
46eafc6
to
de36fb8
Compare
new_id_dict = orig_id.to_dict() | ||
orig_id_keys = orig_id.id_keys | ||
for query_key, query_val in query_dict.items(): | ||
# XXX: What if the query_val is a list? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my main task remaining. I'm really not sure how I want to treat this. If you ask for a composite as DataQuery(name="some_comp", resolution=[500, 1000])
, what do you set in the DataID
? This is before we know what dependencies were found. So does the composite DataID
become DataID(name="some_comp")
with no resolution, resolution 500, or resolution 1000?
Disclaimer: This PR's implementation is very rough at the time of writing.
This PR includes major changes to key parts of Satpy in order to resolve some inconsistencies noticed by users over the years. The high-level concepts that have been changed or updated are:
DataQuery(name="comp", resolution=500), DataQuery(name="comp", resolution=1000)
).Remaining work
Hindsight
For high-level change 1 above, I'm starting to think this was the wrong change or at least that the previous behavior had a good point. That is, if a user creates a query with a lot of keys to apply to many DataIDs from different sources, then not all DataIDs should be required to have all those keys. For example, if I specify a polarization in my query, then I don't think all composites or rather composite dependencies would be able to match that. There are currently no tests to verify this.
AUTHORS.md
if not there already