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 RichDEM from source, moved utility functions to conftest.py #612

Merged
merged 4 commits into from
Oct 28, 2024

Conversation

vschaffn
Copy link
Contributor

@vschaffn vschaffn commented Oct 23, 2024

Description

This PR resolves the ticket #558 that requires the removal of the RichDEM dependency from the core functionnality of the xDEM project to ensure the project's license is not impacted by RichDEM's GNU GPL v3.0 license.

The changes made in this PR remove RichDEM from operational code and ensure that RichDEM is only used for testing purposes. Additionally, documentation and contribution guides have been updated to reflect this adjustment.

Changes Implemented

Dependency Management

Updated setup.cfg and dev-environment.yml to move the richdem package from the main optdependencies to the test section.

Source modifications

  • In terrain.py and dem.py, the use_richdem parameter was removed from functions.
  • Any internal helper functions that directly used RichDEM were completely removed.

Tests adjustments

RichDEM is still used in tests to ensure that all attribute functions give the same results as those of RichDEM. The corresponding part of the code that has been removed from source was transfered to conftest.py.

Documentation Updates

  • Updated documentation to reflect the removal of the use_richdem parameter from functions
  • Added instructions in the contribution gide stating that RichDEM should only be used in tests, and core functionality of xdem must not rely on RichDEM to comply with the license.

Additional informations

Resolves #373

Copy link

@adebardo adebardo left a comment

Choose a reason for hiding this comment

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

It seems that there are still occurrences of 'richdem' in the docstrings in the terrain.py file.

README.md Outdated Show resolved Hide resolved
tests/test_terrain.py Outdated Show resolved Hide resolved
@adebardo
Copy link

The file dev-environment.yml is missing some modifications.

@vschaffn vschaffn force-pushed the 558-delete_richdem_from_source branch from 3a40b4d to 2b4a009 Compare October 23, 2024 14:59
@adebardo
Copy link

@duboise-cnes @adehecq @rhugonnet it's okay for me, what about you ?

@rhugonnet
Copy link
Contributor

Amazing! I did not pick up on anything in the code 🙂.

My only remark: I agree we should state somewhere that we use RichDEM only for tests to clarify the link in terms of license, but I wonder if the README is the best place for this. Is there another file where this could fit (linked to license and dependencies)?
If not, then maybe we could move it to a new section of the README that we create at the bottom to have these kind of statements, could be named "Dependencies and license", or "Testing dependencies", or similar?
The reasoning behind this is that the text at the top is a broad summary for a first reader/potential new user of the package, so we probably don't want to get into too much detail of testing dependencies there!

@rhugonnet
Copy link
Contributor

Additionally, I'm noticing that this PR also resolves #373, can be added to the PR description to close it automatically alongside #558 😉

@duboise-cnes
Copy link
Member

I have looked quickly during vacations :) seems ok for me too.

As romain said, not needeed in the readme, it is more internal of the project, no need to expose it in the main entrance. Not sure it is needeed in the contributing part either
Maybe only in a doctype in conftest.py or in a NOTICE file at Root level ?

Anyway, if only used in tests, there is no so much problem left, this is the more important.
Choose what you think is best with romain and Amaury

Thanks for the PR

@vschaffn vschaffn force-pushed the 558-delete_richdem_from_source branch from 2b4a009 to e8bb887 Compare October 24, 2024 12:40
@vschaffn
Copy link
Contributor Author

@rhugonnet @duboise-cnes Thank you for your feedback. As a NOTICE file will be added with PR #617 I have deleted the changes in the README. Romain and Amaury, don't hesitate to let me know if you want to delete the changes in the contributing part too.

@rhugonnet
Copy link
Contributor

Perfect for a NOTICE file! I agree to also remove from CONTRIBUTING.
I think contributors will implicitly understand that adding a new dependency to the package is a big deal. Or we could clarify this in a generic manner somewhere in CONTRIBUTING for all purposes (but that could be in a later PR).

@vschaffn vschaffn changed the base branch from branch-poc to main October 28, 2024 09:22
@vschaffn vschaffn changed the title [POC] 558 delete richdem from source Remove RichDEM from source, moved utility functions to conftest.py Oct 28, 2024
@vschaffn vschaffn force-pushed the 558-delete_richdem_from_source branch from e8bb887 to 54a5992 Compare October 28, 2024 14:30
@vschaffn vschaffn force-pushed the 558-delete_richdem_from_source branch from 54a5992 to 256c2e6 Compare October 28, 2024 15:12
@adebardo adebardo merged commit 5bde36e into GlacioHack:main Oct 28, 2024
22 checks passed
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.

Silent richdem's messages in xdem.terrain?
4 participants