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

Custom dependencies #178

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

feefladder
Copy link
Contributor

@feefladder feefladder commented Mar 24, 2021

  • Closes #xxxx
  • Tests added
  • Passes black . && flake8
  • Fully documented, including whats-new.rst for all changes and api.rst for new API

added custom dependencies #177, #164
I also added logic for drop_processes, because otherwise that would break. (A bit stubborn of me, apologies 😋 )

@feefladder
Copy link
Contributor Author

I think full documentation should be another PR when all changes are made #177

Comment on lines 344 to 350
model = xs.Model(
{"a": A, "b": B, "c": C, "d": D},
custom_dependencies={"d": "c", "c": "b", "b": "a"},
)
model = model.drop_processes(["b", "c"])
assert model.dependent_processes["d"] == ["a"]

Copy link
Contributor Author

@feefladder feefladder Mar 26, 2021

Choose a reason for hiding this comment

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

I improved this test in #183, apologies for the confusion. It does not test a branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it here

feefladder pushed a commit to feefladder/xarray-simlab that referenced this pull request Apr 6, 2021
@benbovy
Copy link
Member

benbovy commented Jul 24, 2021

Just to say that I'm sorry for the long wait and for my absence of reaction so far @joeperdefloep! I've been struggling these last months to get to review your PRs, to test it and to ensure that we're using the right approach, as the changes here touch one of the core design concepts in xarray-simlab.

I hope to be able to do this properly during the next couple of months.

@feefladder
Copy link
Contributor Author

feefladder commented Aug 2, 2021

Yes, that would be super nice! I'd LOVE not having to work off my own dev branch anymore :) especially if I am going to integrate LandLab in some way (would be exciting!)

Also the biggest part is in strict-check, in terms of reviewing complexity. Most of everything I implemented is a Depth-First Search (DFS) in different places.. The docs explain strict checking, which is the most important piece on how everything is implemented. Let me know if you run into anything!

Edit: the tests are a bit ugly atm in the sense that I make a lot of classes in each test, but having these globally would also be weird, insights on that would be appreciated!

@feefladder
Copy link
Contributor Author

feefladder commented Nov 4, 2021

Hey @benbovy just another message that this review would be very nice! Now my university is looking into reworking some of their erosion models, and this package is of interest!

Also, I could maybe split the DFS and tests from removing processes...

@benbovy
Copy link
Member

benbovy commented Nov 10, 2021

Hi @joeperdefloep, yes sorry (again), this is still on my radar. I've been busy with other projects, notably a big refactor in Xarray to allow custom and explicit indexes, but I should have more time once the Xarray related PR is merged. Thank you for your patience!

@feefladder
Copy link
Contributor Author

No need to be sorry, it sounds like you are doing great work! I now found out how to point to my branch in environment.yml files, so the biggest issue for me is now also resolved, albeit that it is not superduper elegant still :')

@feefladder
Copy link
Contributor Author

Just popped to my mind that using a BFS would probably make the logic easier. Also now I use git-branchless which makes splitting work into branches a lot easier. I could even split out the DFS's from the remove functions if wanted :)

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