Add coords argument to pymc.set_data #5588

soma2000-lang · 2022-03-12T14:29:31Z

soma2000-lang · 2022-03-12T14:30:01Z

@OriolAbril Please have a look into this

michaelosthege · 2022-03-12T17:43:44Z

pymc/model.py

@@ -1728,7 +1728,7 @@ def point_logps(self, point=None, round_vals=2):
 Model._context_class = Model


-def set_data(new_data, model=None):
+def set_data(new_data, coords,model=None):


Don't change the signature

Suggested change

def set_data(new_data, coords,model=None):

def set_data(new_data, model=None, *, coords=None):

Also please make sure to run pre-commit run --all before uploading to avoid unnecessary CI runs due to formatting problems.

michaelosthege · 2022-03-12T17:45:40Z

pymc/model.py

@@ -1766,7 +1766,7 @@ def set_data(new_data, model=None):

    for variable_name, new_value in new_data.items():
        model.set_data(variable_name, new_value)
-
+    return model.set_data(new_data, coords)


Instead of this, you'll need to edit the previous line.

Also make sure to add a test

Ok i will do this changes at the earliest

soma2000-lang · 2022-03-12T20:58:59Z

@michaelosthege Sorry for the silly question but can you please tell specifically what test are you talking about

ricardoV94 · 2022-03-12T21:02:39Z

@michaelosthege Sorry for the silly question but can you please tell specifically what test are you talking about

When you change the behavior of a function or fix a bug you should add a test that shows the new behavior is the expected one. This also ensures future changes to the codebase won't reintroduce the same bugs again.

soma2000-lang · 2022-03-12T21:42:03Z

@ricardoV94 So should I write the test for this , in the tests folder of this repo??

michaelosthege · 2022-03-13T08:56:20Z

@ricardoV94 So should I write the test for this , in the tests folder of this repo??

Use the search function (Ctrl+Shift+F in VSCode) to find occurrences of set_data(. Some of those will be in the test suite (probably test_model.py) and you should put the new test case right next to the existing ones.

soma2000-lang · 2022-03-13T09:01:13Z

Ok

OriolAbril

Hi @soma2000-lang. I'll try to be as detailed as possible in the review, I hope it helps. As always, don't hesitate to ask questions here and on discourse. We do not know what your background is nor how much experience you have using pymc, using python, using pytest... so we can only assume. We might be overly explicit or overly cryptic and can only improve that from interacting and if you ask questions and give detailed answers of what you have tried and what you don't understand.

This PR is adding new functionality to the PyMC library. Therefore, as you saw in the checklists of the PR template it needs to also add tests covering this new functionality. You can remove items from the checklist when they don't apply to your PR but you should only do so if you are sure they do not apply to your case. Otherwise you should keep them, especially when starting to contribute, they are thought to help both you in setting the PR and to help reviewers make sure we don't miss anything.

You can see for example in #4759 which I submitted a while ago how I removed some of the checklist points because that PR did changes to the documentation only and introduced no new functionality. Unlike here, the test check did not apply to that PR.

In this particular case, the test should be added somewhere in https://github.com/pymc-devs/pymc/blob/main/pymc/tests/test_data_container.py, where you will see there are already other tests checking the behaviour of both the set_data function and the set_data method. The first step is making sure you understand how both work and what is the change you are introducing. Once you do you should generate an example that doesn't work currently on pymc and will work on your PR thanks to your changes. If you have never used pymc.set_data let us know and we'll search some examples.

I will also leave a comment on the issue about collaborating and avoiding duplicated work, especially now that we are working on the next major release and there are also many prospective gsoc applicants contributing.

OriolAbril · 2022-03-15T11:32:16Z

pymc/model.py

@@ -1765,8 +1765,8 @@ def set_data(new_data, model=None):
    model = modelcontext(model)

    for variable_name, new_value in new_data.items():
-        model.set_data(variable_name, new_value)
-
+        model.set_data(variable_name, new_value,coords)


Suggested change

model.set_data(variable_name, new_value,coords)

model.set_data(variable_name, new_value, coords=coords)

Here I think it would be best to use the argument as keyword instead of positional, but that should not change the output.

OriolAbril · 2022-03-15T11:35:29Z

pymc/model.py

-        model.set_data(variable_name, new_value)
-
+        model.set_data(variable_name, new_value,coords)
+    """return model.set_data(new_data, coords)"""


This line should be removed. This function modifies the data stored in the model class and does not have and should not have any return function. If you look at the code of pymc.Model.set_data you'll see that it doesn't have any return value either.

Adding this as a string makes the code run because this line is now ignored instead of being executed, so it should be removed to avoid adding code that is never executed.

I would definitely do the required changes and keep in mind what ever you have said

OriolAbril · 2022-03-22T10:48:50Z

Do you need any help with tests @soma2000-lang ?

soma2000-lang · 2022-03-27T11:04:28Z

@OriolAbril
Actually yes
Sorry for the very late reply But I was going through unit testing in general
: But I am kinda confused how to do test for large codespace like this
Can you give me some references or examples that would be great

OriolAbril · 2022-03-27T18:07:01Z

Have you gone over the tests in test_data_container.py file that I linked in the previous comment? Specifically the ones using set_data (in either two of its forms)? Do you understand what they are doing?

soma2000-lang · 2022-03-29T23:30:34Z

@OriolAbril I had went over the tests ,its bascically testing with some sample data and checking it with against some cases

OriolAbril · 2022-03-30T16:30:15Z

Do you think you can use these tests as a template and write a new one that covers the use of coords in pymc.set_data?

soma2000-lang · 2022-03-30T16:55:58Z

@OriolAbril Yes I will try doing that.

OriolAbril · 2022-04-11T21:26:46Z

Any news? Have you been able to try that? If so where are you stuck?

I am happy to help but can't do anything if I don't know where and how I'm supposed to do it. I'd like to use this in pymc-devs/pymc-examples#285 and will probably take over in ~2 weeks if still pending.

soma2000-lang · 2022-04-11T21:55:41Z

Sorry for the inconvinience due to ongoing University examination I was a bit busy and unable to work on it but I will resume soon

…

On Tue, Apr 12, 2022, 02:56 Oriol Abril-Pla ***@***.***> wrote: Any news? Have you been able to try that? If so where are you stuck? I am happy to help but can't do anything if I don't know where and how I'm supposed to do it. I'd like to use this in pymc-devs/pymc-examples#285 <pymc-devs/pymc-examples#285> and will probably take over in ~2 weeks if still pending. — Reply to this email directly, view it on GitHub <#5588 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANLS36KEVWONGXVETHEV533VESKKBANCNFSM5QR6UULA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

soma2000-lang · 2022-04-12T07:30:52Z

@OriolAbril

pymc/model.py

OriolAbril · 2022-04-12T11:22:20Z

Sorry for the inconvinience due to ongoing University examination I was a bit busy and unable to
work on it but I will resume soon

Great, looking forward to the tests

codecov · 2022-04-23T23:08:40Z

Codecov Report

Merging #5588 (654418b) into main (162ad6c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5588      +/-   ##
==========================================
+ Coverage   88.90%   88.92%   +0.01%     
==========================================
  Files          75       75              
  Lines       13738    13738              
==========================================
+ Hits        12214    12216       +2     
+ Misses       1524     1522       -2

Impacted Files	Coverage Δ
pymc/model.py	`86.63% <100.00%> (+0.27%)`	⬆️

OriolAbril · 2022-04-24T10:04:32Z

Thanks for getting this started @soma2000-lang, I added the test to get the PR merged as having this will make my life much easier. Happy to go over the test if something were not clear or help you find other issues to work on.

* Refactor get_tau_sigma to handle lists * Fixed syntax error * Black formatted * Change assertions to ValueError raises * Prior predictions constant data (#5723) * support return constant_data with prior pred * pre-commit format * added test * code review * Fix issue probably-meant-fstring found at https://codereview.doctor (#5726) * Add coords argument to pymc.set_data (#5588) Co-authored-by: Oriol (ZBook) <[email protected]> * remove MultinomialRV override * ⬆️ UPGRADE: Autoupdate pre-commit config * Group GaussianRandomWalk tests in single class * Infer steps from shape in GaussianRandomWalk * Unpin upper limit on numpydoc Closes #5401 * ⬆️ UPGRADE: Autoupdate pre-commit config * Standardize docstrings of input dists arguments and add warning about cloning * Remove unnecessary tag in Simulator logp * Replace deprecated tag.ignore_logprob * Group compile_pymc tests in own class * Remove remaining uses of default_updates in codebase * Remove redundant/wrong docstrings from GaussianRandomWalk logp * Add moment to GaussianRandomWalk and fix mu/sigma broadcasting bug * Do not create temporary SymbolicDistribution just to retrieve number of RNGs needed Reordered methods for consistency * Move SymbolicDistribution docstring to body of class * Refactor AR distribution * Deprecates AR1 distribution * Implements random and moment methods * Batching works on the left as with other distributions * Rename `pandas_to_array` to `convert_observed_data` * Obtain step information from dims and observed * Make AR steps extend shape beyond initial_dist This is consistent with the meaning of steps in the GaussianRandomWalk and translates directly to the number of scan steps taken Co-authored-by: TimOliverMaier <[email protected]> Co-authored-by: code-review-doctor <[email protected]> Co-authored-by: Somasree Majumder <[email protected]> Co-authored-by: Oriol (ZBook) <[email protected]> Co-authored-by: danhphan <[email protected]> Co-authored-by: twiecki <[email protected]> Co-authored-by: Ricardo <[email protected]> Co-authored-by: Ravin Kumar <[email protected]> Co-authored-by: Michael Osthege <[email protected]>

michaelosthege requested changes Mar 12, 2022

View reviewed changes

soma2000-lang changed the title ~~Aadd coords argument to pymc.set_data~~ Add coords argument to pymc.set_data Mar 12, 2022

OriolAbril reviewed Mar 15, 2022

View reviewed changes

OriolAbril mentioned this pull request Mar 15, 2022

add coords argument to pymc.set_data #5547

Closed

review-notebook-app bot mentioned this pull request Apr 11, 2022

Updated multilevel modeling to v4 pymc-devs/pymc-examples#285

Merged

twiecki reviewed Apr 12, 2022

View reviewed changes

pymc/model.py Outdated Show resolved Hide resolved

twiecki reviewed Apr 12, 2022

View reviewed changes

pymc/model.py Outdated Show resolved Hide resolved

soma2000-lang and others added 5 commits April 24, 2022 01:51

Aadd coords argument to pymc.set_data

e9bf9f9

Adding

7a1c71d

Update model.py

e8fe4a0

Update model.py

855f55c

add test and run black

654418b

OriolAbril approved these changes Apr 23, 2022

View reviewed changes

michaelosthege approved these changes Apr 24, 2022

View reviewed changes

michaelosthege merged commit bae5087 into pymc-devs:main Apr 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add coords argument to pymc.set_data #5588

Add coords argument to pymc.set_data #5588

soma2000-lang commented Mar 12, 2022

soma2000-lang commented Mar 12, 2022

michaelosthege Mar 12, 2022

michaelosthege Mar 12, 2022

soma2000-lang Mar 12, 2022

soma2000-lang commented Mar 12, 2022

ricardoV94 commented Mar 12, 2022 •

edited

Loading

soma2000-lang commented Mar 12, 2022

michaelosthege commented Mar 13, 2022

soma2000-lang commented Mar 13, 2022

OriolAbril left a comment

OriolAbril Mar 15, 2022

OriolAbril Mar 15, 2022

soma2000-lang Mar 15, 2022

OriolAbril commented Mar 22, 2022

soma2000-lang commented Mar 27, 2022 •

edited

Loading

OriolAbril commented Mar 27, 2022

soma2000-lang commented Mar 29, 2022 •

edited

Loading

OriolAbril commented Mar 30, 2022

soma2000-lang commented Mar 30, 2022

OriolAbril commented Apr 11, 2022

soma2000-lang commented Apr 11, 2022 via email •

edited

Loading

soma2000-lang commented Apr 12, 2022

OriolAbril commented Apr 12, 2022

codecov bot commented Apr 23, 2022 •

edited

Loading

OriolAbril commented Apr 24, 2022

	def set_data(new_data, coords,model=None):
	def set_data(new_data, model=None, *, coords=None):

	model.set_data(variable_name, new_value,coords)
	model.set_data(variable_name, new_value, coords=coords)

Add coords argument to pymc.set_data #5588

Add coords argument to pymc.set_data #5588

Conversation

soma2000-lang commented Mar 12, 2022

soma2000-lang commented Mar 12, 2022

michaelosthege Mar 12, 2022

Choose a reason for hiding this comment

michaelosthege Mar 12, 2022

Choose a reason for hiding this comment

soma2000-lang Mar 12, 2022

Choose a reason for hiding this comment

soma2000-lang commented Mar 12, 2022

ricardoV94 commented Mar 12, 2022 • edited Loading

soma2000-lang commented Mar 12, 2022

michaelosthege commented Mar 13, 2022

soma2000-lang commented Mar 13, 2022

OriolAbril left a comment

Choose a reason for hiding this comment

OriolAbril Mar 15, 2022

Choose a reason for hiding this comment

OriolAbril Mar 15, 2022

Choose a reason for hiding this comment

soma2000-lang Mar 15, 2022

Choose a reason for hiding this comment

OriolAbril commented Mar 22, 2022

soma2000-lang commented Mar 27, 2022 • edited Loading

OriolAbril commented Mar 27, 2022

soma2000-lang commented Mar 29, 2022 • edited Loading

OriolAbril commented Mar 30, 2022

soma2000-lang commented Mar 30, 2022

OriolAbril commented Apr 11, 2022

soma2000-lang commented Apr 11, 2022 via email • edited Loading

soma2000-lang commented Apr 12, 2022

OriolAbril commented Apr 12, 2022

codecov bot commented Apr 23, 2022 • edited Loading

Codecov Report

OriolAbril commented Apr 24, 2022

ricardoV94 commented Mar 12, 2022 •

edited

Loading

soma2000-lang commented Mar 27, 2022 •

edited

Loading

soma2000-lang commented Mar 29, 2022 •

edited

Loading

soma2000-lang commented Apr 11, 2022 via email •

edited

Loading

codecov bot commented Apr 23, 2022 •

edited

Loading