Improving the behaviour of an empty InferenceData object. (Issue no 486). #577

Ban-zee · 2019-01-31T19:44:39Z

Addressing issue #486

canyon289 · 2019-01-31T21:53:00Z

@Ban-zee Please add tests as well

ColCarroll · 2019-02-01T14:27:09Z

Thanks for this contribution! A few suggestions:

It is helpful if the title and description say what the pull request is doing -- linking to the issue it fixes is helpful, but being able to search for things in the future, as well as having meaningful git commit messages is important.
I would make this change to the InferenceData object directly

Ban-zee · 2019-02-01T18:13:46Z

Definitely, will keep that in mind in the future. I'll make the necessary changes.

canyon289 · 2019-02-04T17:54:55Z

@Ban-zee If you want to practice you can change the title of this PR and amend your commit to have a more useful message :)

Ban-zee · 2019-02-05T20:25:41Z

A minor question, exactly what test should I include as the test for checking an inference object's existence is already included in test_data.py. TIA.

canyon289 · 2019-02-06T01:55:18Z

I would add a meaningful test for these lines that are currently not tested
https://coveralls.io/builds/21387295/source?filename=arviz%2Fdata%2Fio_netcdf.py#L47

Reading this over a bit more too can we add a doc string that briefly explains why the if else is there?

To consolidate comments here's what I would like for an approval

Change PR name to a human understandable one
Change git commit message to be human understandable
Add test for else block
Add comment explaining case that if else block is handling

canyon289 · 2019-02-09T02:46:52Z

@Ban-zee Thanks for the title change :)
If you need help with the other asks let us know! We're here to help

Ban-zee · 2019-02-09T10:49:25Z

Sorry for keeping this PR open for so long, I was a bit busy this past week. Regarding the addition of tests, it would be helpful if anyone could guide me through that process.

…

On Sat, Feb 9, 2019 at 8:16 AM Ravin Kumar ***@***.***> wrote: @Ban-zee <https://github.com/Ban-zee> Thanks for the title change :) If you need help with the other asks let us know! We're here to help — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#577 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AcC4Dihks0NcPS2Fngcm5gPDflIueptbks5vLjadgaJpZM4adBGQ> .

canyon289 · 2019-02-09T15:36:36Z

@Ban-zee Don't worry about how long you keep the PR open, there's no timeline in open source!

For testing, are you familiar with pytest or testing principles in general? Trying to get a baseline of where you're at so I can guide you appropriately

Ban-zee · 2019-02-09T22:49:55Z

Thinking of pushing my commits with the other changes you suggested(other than the tests), but git keeps rejecting it as the tip of my local branch is behind the remote. Tried resolving it but nothing seem to work. Should I just force push? Regarding the tests, I have gone through the pytest documentation and have looked into the tests included in this project. It would be helpful if you could tell me how to add my tests to the ones which already exist (in my case mainly for the else block).

…

On Sat, Feb 9, 2019 at 9:06 PM Ravin Kumar ***@***.***> wrote: @Ban-zee <https://github.com/Ban-zee> Don't worry about how long you keep the PR open, there's no timeline in open source! For testing, are you familiar with pytest or testing principles in general? Trying to get a baseline of where you're at so I can guide you appropriately — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#577 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AcC4Dj8-kbeDJmlateu5a3d55jBmelOpks5vLusFgaJpZM4adBGQ> .

canyon289 · 2019-02-10T08:56:32Z

When you do a "destructive" git command like amend you have to force push. Just be careful because if you mess up the command its becomes more difficult to go back, and in some cases impossible.

For the data functions I would look at this module and this test. It'll give you an idea how things are implemented

https://github.com/arviz-devs/arviz/blob/master/arviz/tests/test_data.py#L651

ahartikainen · 2019-02-10T10:09:21Z

Let's say it is impossible. (It is just really really hard, but doable)

Ban-zee · 2019-02-11T18:50:25Z

I did use amend. I'll have to force push anyway now.

…

On Sun, Feb 10, 2019 at 3:39 PM Ari Hartikainen ***@***.***> wrote: Let's say it is impossible. (It is just really really hard, but doable) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#577 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AcC4DkVoMGt6OE1zRaBHpqNkOb6NZxO-ks5vL-_SgaJpZM4adBGQ> .

* Made changes to inference_data.py directly instead of io_netcdf.py as suggested. * Added comments. * Not included the tests yet!

*Not included tests yet!!

Ban-zee · 2019-02-18T19:12:57Z

Getting the following error: "No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself." Any idea on what should be done?

ahartikainen · 2019-02-18T20:27:41Z

That happens sometimes (travis freezes). Restarting the job usually works (might need admin rights, so usually someone from the devteam can do it).

Ban-zee · 2019-02-18T20:57:05Z

Thank you.

ColCarroll · 2019-02-18T21:12:16Z

Hrm... fails on Python 3.5, but not Python 3.6. I restarted in case it is a flaky test, but here's the message:

=================================== FAILURES ===================================
______________________ TestIONetCDFUtils.test_io_function ______________________
self = <arviz.tests.test_data.TestIONetCDFUtils object at 0x7f4bfb122668>
data = <class 'arviz.tests.test_data.TestIONetCDFUtils.data.<locals>.Data'>
eight_schools_params = {'J': 8, 'sigma': array([15., 10., 16., 11.,  9., 11., 10., 18.]), 'y': array([28.,  8., -3.,  7., -1.,  1., 18., 12.])}
    def test_io_function(self, data, eight_schools_params):
        inference_data = self.get_inference_data(  # pylint: disable=W0612
            data, eight_schools_params
        )
        assert hasattr(inference_data, "posterior")
        here = os.path.dirname(os.path.abspath(__file__))
        data_directory = os.path.join(here, "saved_models")
        filepath = os.path.join(data_directory, "io_function_testfile.nc")
        # az -function
        to_netcdf(inference_data, filepath)
        assert os.path.exists(filepath)
        assert os.path.getsize(filepath) > 0
        inference_data2 = from_netcdf(filepath)
>       assert hasattr(inference_data2, "posterior")
E       AssertionError: assert False
E        +  where False = hasattr(Inference data with groups:\n	> posterior_predictive, 'posterior')

Ban-zee · 2019-02-18T21:41:33Z

Any idea on why the test fails in python 3.5 but not in 3.6?

ahartikainen · 2019-02-18T23:04:40Z

arviz/data/inference_data.py

+                data.to_netcdf(filename, mode=mode, group=group, **kwargs)
+                data.close()
+                mode = "a"
+                return filename


here is indentation error.

You could return filename at the end of the function.

ahartikainen · 2019-02-18T23:05:28Z

arviz/data/inference_data.py

+        else:  # creates a netcdf file for an empty InferenceData object.
+            empty_netcdf_file = nc.Dataset(filename, mode="w", format="NETCDF4")
+            empty_netcdf_file.close()
+            return filename


remove the first return filename and return this outside the if-else block. (indent back)

Ban-zee · 2019-02-19T20:08:22Z

Made the changes. Thank you for your help. Are there any more changes to be made prior to merging?

Ban-zee · 2019-02-21T02:18:09Z

Any other change I have to make before merging?

ColCarroll · 2019-02-21T02:35:24Z

This looks good -- thank you for the contribution @Ban-zee!

Ban-zee changed the title ~~Issue no #486~~ Improving the behaviour of an empty InferenceData object. (Issue no 486). Feb 6, 2019

canyon289 mentioned this pull request Feb 17, 2019

Added documentation example for combining InferenceData (#552) and also solving the issue of creaton of empty file for better behaviour with inference data(#486) #590

Closed

Ban-zee force-pushed the i-486 branch from 3ca56d9 to 43d9789 Compare February 18, 2019 13:07

Ban-zee added 4 commits February 18, 2019 18:38

Issue no arviz-devs#486

de81047

Made the following changes(arviz-devs#486):

06d1729

* Made changes to inference_data.py directly instead of io_netcdf.py as suggested. * Added comments. * Not included the tests yet!

*Resolved merge changes

43d9789

*Not included tests yet!!

Added the test

cc51d89

ahartikainen reviewed Feb 18, 2019

View reviewed changes

moved the return statement outside the if-else block

9bee542

ColCarroll merged commit 000b282 into arviz-devs:master Feb 21, 2019

ColCarroll mentioned this pull request Feb 22, 2019

Bump minor version #596

Merged

canyon289 mentioned this pull request Mar 8, 2019

Better behavior with empty InferenceData #486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving the behaviour of an empty InferenceData object. (Issue no 486). #577

Improving the behaviour of an empty InferenceData object. (Issue no 486). #577

Ban-zee commented Jan 31, 2019

canyon289 commented Jan 31, 2019

ColCarroll commented Feb 1, 2019

Ban-zee commented Feb 1, 2019

canyon289 commented Feb 4, 2019 •

edited

Loading

Ban-zee commented Feb 5, 2019

canyon289 commented Feb 6, 2019 •

edited

Loading

canyon289 commented Feb 9, 2019

Ban-zee commented Feb 9, 2019 via email

canyon289 commented Feb 9, 2019

Ban-zee commented Feb 9, 2019 via email

canyon289 commented Feb 10, 2019

ahartikainen commented Feb 10, 2019

Ban-zee commented Feb 11, 2019 via email

Ban-zee commented Feb 18, 2019

ahartikainen commented Feb 18, 2019

Ban-zee commented Feb 18, 2019

ColCarroll commented Feb 18, 2019

Ban-zee commented Feb 18, 2019

ahartikainen Feb 18, 2019

ahartikainen Feb 18, 2019 •

edited

Loading

Ban-zee commented Feb 19, 2019 •

edited

Loading

Ban-zee commented Feb 21, 2019

ColCarroll commented Feb 21, 2019

Improving the behaviour of an empty InferenceData object. (Issue no 486). #577

Improving the behaviour of an empty InferenceData object. (Issue no 486). #577

Conversation

Ban-zee commented Jan 31, 2019

canyon289 commented Jan 31, 2019

ColCarroll commented Feb 1, 2019

Ban-zee commented Feb 1, 2019

canyon289 commented Feb 4, 2019 • edited Loading

Ban-zee commented Feb 5, 2019

canyon289 commented Feb 6, 2019 • edited Loading

canyon289 commented Feb 9, 2019

Ban-zee commented Feb 9, 2019 via email

canyon289 commented Feb 9, 2019

Ban-zee commented Feb 9, 2019 via email

canyon289 commented Feb 10, 2019

ahartikainen commented Feb 10, 2019

Ban-zee commented Feb 11, 2019 via email

Ban-zee commented Feb 18, 2019

ahartikainen commented Feb 18, 2019

Ban-zee commented Feb 18, 2019

ColCarroll commented Feb 18, 2019

Ban-zee commented Feb 18, 2019

ahartikainen Feb 18, 2019

Choose a reason for hiding this comment

ahartikainen Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

Ban-zee commented Feb 19, 2019 • edited Loading

Ban-zee commented Feb 21, 2019

ColCarroll commented Feb 21, 2019

canyon289 commented Feb 4, 2019 •

edited

Loading

canyon289 commented Feb 6, 2019 •

edited

Loading

ahartikainen Feb 18, 2019 •

edited

Loading

Ban-zee commented Feb 19, 2019 •

edited

Loading