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

Fix #181 #183 #184 #185

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Fix #181 #183 #184 #185

merged 4 commits into from
Jan 7, 2025

Conversation

nkarasiak
Copy link

Hi @Kirill888,

As suggested, I improved my pull request by providing 3 differents commits for the 3 differents issues that has been fixed.
You can take a look at it, it's way clearer now :)
Thanks !

@@ -107,13 +107,13 @@ def test_load_from_json_stackstac(fake_dask_client, bench_site1, bench_site2):
resampling="nearest",
extra={
"odc-stac": {"groupby": "solar_day", "stac_cfg": CFG},
"stackstac": {"dtype": "uint16", "fill_value": 0},
Copy link
Member

Choose a reason for hiding this comment

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

can you explain why the change of dtype please?

Copy link
Member

Choose a reason for hiding this comment

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

it significantly increases memory requirement (4 times)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @Kirill888 ,
It's because the uint16 values on stackstac don't support (anymore?) the fill_value=0 , so the easiest way to deal with that was to change to int64.
ValueError: The fill_value 0 is incompatible with the output dtype uint16. Either use `dtype='int64'`, or pick a different `fill_value`.

Copy link
Member

Choose a reason for hiding this comment

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

what version of stackstac and numpy is that? My main concern here is that this change puts stackstac at disadvantage. Also this error looks to me like a numpy 2 upgrade issue.

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.54%. Comparing base (d7cdd5c) to head (9fa4966).
Report is 100 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #185      +/-   ##
===========================================
+ Coverage    87.58%   91.54%   +3.95%     
===========================================
  Files           23       24       +1     
  Lines         1982     2851     +869     
===========================================
+ Hits          1736     2610     +874     
+ Misses         246      241       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kirill888 Kirill888 force-pushed the fixes branch 2 times, most recently from a2442d3 to f467fc6 Compare January 7, 2025 01:50
Copy link
Member

@Kirill888 Kirill888 left a comment

Choose a reason for hiding this comment

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

Thanks @nkarasiak it should all be working now

in the future, please use descriptive commit messages, and do not rely on the issue number alone.

Summary of the fix #IssueNumber

detailed description if needed

When trying to understand what changed it's really hard to remember which one is 181 which is 183 or 184.

@Kirill888 Kirill888 merged commit 9f055a3 into opendatacube:develop Jan 7, 2025
14 checks passed
@nkarasiak
Copy link
Author

Got it @Kirill888 , and thanks for educating me about how to enhance my fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants