Skip to content

Adding function for mutable coords #6515

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 7 commits into from
Feb 16, 2023
Merged

Conversation

Dhruvanshu-Joshi
Copy link
Member

What is this PR about?
In this PR , I have made attempts to incorporate suggestions made from 6497.
Coordinate dimensions by default are assumed to be immutable unless they are explicitly mentioned to be mutable in the add_coord function.
In this PR, we aim to register mutable dimension coordinates with the model without having to explicitly set mutable = True in the add_coord method for each item in coords.

Checklist

Major / Breaking Changes

  • no

New features

  • Added another function called add_mutable_coords which registers new coordinates that are mutable by iterating in a for loop and calling self.add_coord(..., mutable=True) for every iteration.

Bugfixes

  • no

Documentation

  • Documentation has been updated consistent with modifications

Maintenance

  • no

@twiecki
Copy link
Member

twiecki commented Feb 10, 2023

Thanks! We should also change the error message to mention to set mutable=True then.

@twiecki
Copy link
Member

twiecki commented Feb 10, 2023

And I think we should make it mutable by default.

@Dhruvanshu-Joshi
Copy link
Member Author

How would you like to make it mutable by default?
What I was thinking was to create another function called add_mutable_coord which will be same as add_coord but will assume mutable = True. We can also update add_coord to consider only cases when mutable = False. We will also throw error messages when someone tries to add immutable coordinates using add_mutable_coord and vice-versa. Hence by using add_mutable_coords which will call add_mutable_coord, one can add mutable coordinates to the model context and by using add_coords which iteratively calls add_coord, we can add immutable coordinates to the model context.
Should I proceed with this?

@michaelosthege
Copy link
Member

Let's keep it as simple as possible:

  • Introduce coords_mutable to the Model.__init__ signature
  • Put a two-line for iterator right below the self.add_coords(coords) line

There's no need to create another method. Not counting tests this can be a 3-line change!

I'm also against changing defaults because of possible shape or performance issues.

@Dhruvanshu-Joshi
Copy link
Member Author

Hey @twiecki @michaelosthege when we use coords_mutable.items, we are expecting a none-type object to be a dictionary and hence all of the tests are failing. I suggest that we include this loop in an if block which will check
if coords_mutable is not None : similar to how add_coords does it to avoid this issue. Should I proceed with this?

@michaelosthege
Copy link
Member

Hey @twiecki @michaelosthege when we use coords_mutable.items, we are expecting a none-type object to be a dictionary and hence all of the tests are failing. I suggest that we include this loop in an if block which will check if coords_mutable is not None : similar to how add_coords does it to avoid this issue. Should I proceed with this?

sure, it's your PR!

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #6515 (1927320) into main (28bac77) will decrease coverage by 25.22%.
The diff coverage is 33.33%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6515       +/-   ##
===========================================
- Coverage   94.74%   69.52%   -25.22%     
===========================================
  Files         146      146               
  Lines       27807    27810        +3     
===========================================
- Hits        26346    19336     -7010     
- Misses       1461     8474     +7013     
Impacted Files Coverage Δ
pymc/model.py 89.08% <33.33%> (-0.78%) ⬇️
pymc/tests/sampling/test_forward.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/stats/test_convergence.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/distributions/test_bound.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/sampling/test_population.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/step_methods/test_slicer.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/stats/test_log_likelihood.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/step_methods/hmc/test_nuts.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/step_methods/test_compound.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/distributions/test_censored.py 0.00% <0.00%> (-100.00%) ⬇️
... and 43 more

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

Successfully merging this pull request may close these issues.

3 participants