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

Update enso notebook #832

Closed
wants to merge 6 commits into from
Closed

Conversation

lee1043
Copy link
Contributor

@lee1043 lee1043 commented Mar 30, 2022

ENSO demo was updated -- manual installation process for the ENSO metrics by cloning it github repository has been removed.

@lee1043 lee1043 requested a review from acordonez March 30, 2022 19:20
@acordonez
Copy link
Collaborator

@lee1043 looking at the diff it does seem that some of the metrics have changed value slightly - is that expected given the change in datasets?

@@ -801,11 +748,11 @@
" },\n",
" \"metric\": {\n",
" \"ERA-Interim\": {\n",
" \"value\": 0.6404285425003315,\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lee1043 for example

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried a couple of tests locally - ran your new notebook using the pmp_enso_tutorial_files.v20210823.txt data and ensometrics from conda-forge, and also ran the old notebook (in main) with pmp_enso_tutorial_files.v20210823.txt and ensometrics installed locally via pip. In both cases I am seeing the same statistics as the old notebook from the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking. Let me try rerun the notebook in my local.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, if I understand correctly, it seems as though the differences must be coming from a change in the obs. Do I have that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gleckler1 I don't think it is caused by any OBS diff. Now the updated notebook and old notebook are using the same OBS (from the v20210823.txt), and differences in statistics are very small numbers (under point 9th digit), which I think it is negligible. I think the difference is coming from different machines were being used.

I am attaching a file that I got from below commend,
conda list > conda_list.txt
conda_list.txt
@acordonez could you try same on your env and see if there is any difference in versions of the PMP dependencies?

One possibility to keep the change in this PR to be more focused and traceable (most change in this PR is removing enso_metrics's manual installation part from the demo notebook), how about @acordonez uploads the new notebook ran from her local to this PR (or via new PR if that is easier)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gleckler1 @lee1043 Agree that it doesn't seem to be an issue with the obs, since I am getting the same results now that I got when we did the last round of updates. Differences due to machines sounds plausible to me. There are quite a few differences between my environment and Jiwoo's also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can try running the notebook and pushing the results to this PR.

@acordonez
Copy link
Collaborator

@lee1043 I don't think I can push to this branch but I ran your notebook in another branch (832_ao_enso) and opened a PR (#836). You might be able to pull those changes here if we want to keep this PR.

@lee1043
Copy link
Contributor Author

lee1043 commented Apr 2, 2022

@acordonez thank you, I am closing this PR and will merge the new PR (#836).

@lee1043 lee1043 closed this Apr 2, 2022
@lee1043 lee1043 deleted the update_enso_notebook branch April 6, 2022 00:03
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.

None yet

3 participants