Skip to content

Conversation

@mmkekic
Copy link
Collaborator

@mmkekic mmkekic commented Jun 3, 2020

This PR updates the environment packages.

@mmkekic mmkekic requested a review from gonzaponte June 3, 2020 07:53
@mmkekic
Copy link
Collaborator Author

mmkekic commented Jun 3, 2020

6 tests related to deconv_functions and beersheba are failing. I think the main problem is in interpolate_signal in the call of scipy.interpolate.griddata but @gonzaponte and me couldn't figure out why would this change between versions. @Aretno could you check that your algorithm works with updated numpy/scipy? Also, as I understand the interpolation is always done on the regular grid, why aren't you using scipy interpolation methods that work on gridded data?

@Aretno
Copy link
Collaborator

Aretno commented Jun 3, 2020

Will look into it. Regarding the gridded data, no strong reason but that won't be the case when working on 3D as the Z position of the hits is not gridded.

Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

We should have an automated test for this thing. Since we don't we should have a non-automated one at least. Try to run the script both installing and not installing conda. neutrinos would be a good test bench for this as many people will use it there.

@mmkekic mmkekic force-pushed the update-env branch 3 times, most recently from 416ac4a to 8f95580 Compare June 16, 2020 20:12
@gonzaponte gonzaponte mentioned this pull request Jun 21, 2020
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

This PR updates our packages to a recent version, much needed after freezing them for 1.5 years.
There are some caveats:

  • Some tests have been skipped due to a change in behavior of scipy.interpolate that doesn't affect the final result. These are to be fixed soon.
  • Some tests have moved from a single-input to a hypothesis-generated input, which may increase test-execution time (we haven't evaluated it).

Otherwise, this is ready to be merged. Good job.

@gonzaponte
Copy link
Collaborator

Please rebase and merge.

@carmenromo carmenromo merged commit 515a56c into next-exp:master Jul 2, 2020
carmenromo added a commit that referenced this pull request Jul 17, 2020
#733

[author: Aretno]

When updating some packages in #723 there were some test that were
marked as skipped after checking final result did not change
performance-wise. This PR aims to update the values of the test so
they can be enabled again.

[reviewer: gonzaponte]

Scipy interpolate changed behavior in the recent package upgrade. The
exact methodology change hasn't been found in the changelog, which is
annoying. The differences in the values obtained in the tests vary
around the percent level, but the overall performance of the algorithm
hasn't been affected, according to @Aretno. Thus, the update simply
relies on freezing these new values, hoping that they will not change
again in a future release of scipy.
@mmkekic mmkekic deleted the update-env branch February 22, 2021 19:34
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.

4 participants