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

Enhance(+) readme and add .gitignore #15

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Enhance(+) readme and add .gitignore #15

merged 2 commits into from
Oct 11, 2024

Conversation

omad
Copy link
Member

@omad omad commented Oct 11, 2024

Start splitting out useful and easily reviewable parts of the two long standing PRs on this repo.

.gitignore is from the native-load PR
and the README is mostly from the Rust Geomedian PR.

@omad omad requested review from alexgleith and emmaai October 11, 2024 03:51
@omad omad merged commit bd2fb68 into main Oct 11, 2024
1 check passed
@omad omad deleted the dra/admin branch October 11, 2024 04:25
@robbibt
Copy link
Contributor

robbibt commented Oct 11, 2024

@omad Not sure this is the best place, but just a few stray thoughts on this repo:

  • Although this repo contains some fantastic tools, it is difficult to install, which has a big impact on all our downstream packages that use it (e.g. DEA Tools, DEA Intertidal etc). Making the requirements more minimal and/or moving dependencies to optional dependencies would make a big difference
  • Strongly agree that we should deprecate any functionality here that overlaps with odc-geo - I think that datacube, odc-geo and odc-stac should be the point of truth wherever possible
  • The image processing tools in this repo (dilation, erosion) etc are heavily used in downstream repos. Personally I think they would fit nicely in odc-geo as part of a new masking module there, but it would be great to preserve them here in the meantime (even though skimage is a big dependancy, I've usually found it pretty easy to install)

@omad
Copy link
Member Author

omad commented Oct 11, 2024

@robbibt

Not sure this is the best place, but just a few stray thoughts on this repo:

Haha 😉 , probably not the best place, but, a place is important, and I'm always keen for your thoughts Robbi!

Although this repo contains some fantastic tools, it is difficult to install, which has a big impact on all our downstream packages that use it (e.g. DEA Tools, DEA Intertidal etc). Making the requirements more minimal and/or moving dependencies to optional dependencies would make a big difference

Okay, we can absolutely make some dependencies optional. It's easy and a step in the right direction.

It sounds like some deprecating/deleting and restructuring are also in order, which will take a bit more to achieve.

Strongly agree that we should deprecate any functionality here that overlaps with odc-geo - I think that datacube, odc-geo and odc-stac should be the point of truth wherever possible

Thanks!

The image processing tools in this repo (dilation, erosion) etc are heavily used in downstream repos. Personally I think they would fit nicely in odc-geo as part of a new masking module there, but it would be great to preserve them here in the meantime (even though skimage is a big dependancy, I've usually found it pretty easy to install)

That's good to know! I wasn't even aware there were masking tools in odc-geo. 💡

IIRC, we only use a tiny tiny part of skimage, so it might even be worth vendoring that bit, or getting it from elsewhere.

@robbibt
Copy link
Contributor

robbibt commented Oct 11, 2024

That's good to know! I wasn't even aware there were masking tools in odc-geo. 💡

There isn't "yet". But there are discussions around doing it here: opendatacube/odc-geo#117

(would be particularly nice as masking funcs would then be something you could access via odc-geo's .odc. accessor, e.g. ds.odc.mask_invalid_data() etc instead of calling in external funcs from odc-algo or datacube-core)

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.

3 participants