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

histogramdd normed argument replaced by density #3976

Merged
merged 7 commits into from
Dec 23, 2022
Merged

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Dec 20, 2022

Fixes #3975

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Base: 93.67% // Head: 93.48% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (de037ca) compared to base (d8526ff).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3976      +/-   ##
===========================================
- Coverage    93.67%   93.48%   -0.19%     
===========================================
  Files           14      190     +176     
  Lines         1091    24951   +23860     
  Branches         0     3527    +3527     
===========================================
+ Hits          1022    23326   +22304     
- Misses          69     1102    +1033     
- Partials         0      523     +523     
Impacted Files Coverage Δ
package/MDAnalysis/analysis/density.py 100.00% <ø> (ø)
package/MDAnalysis/topology/tpr/utils.py 95.48% <0.00%> (ø)
package/MDAnalysis/analysis/rms.py 93.02% <0.00%> (ø)
package/MDAnalysis/lib/distances.py 97.62% <0.00%> (ø)
package/MDAnalysis/topology/GMSParser.py 95.00% <0.00%> (ø)
package/MDAnalysis/topology/MOL2Parser.py 98.24% <0.00%> (ø)
package/MDAnalysis/lib/mdamath.py 100.00% <0.00%> (ø)
package/MDAnalysis/converters/RDKitParser.py 96.96% <0.00%> (ø)
.../MDAnalysis/analysis/encore/clustering/__init__.py 100.00% <0.00%> (ø)
package/MDAnalysis/topology/__init__.py 100.00% <0.00%> (ø)
... and 167 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pgbarletta
Copy link
Contributor

I think you're just missing this normed. And this one on the docs.

@@ -18,6 +18,8 @@ The rules for this file:
* 2.5.0

Fixes
* np.histogramdd calls in :class:`DensityAnalysis` now pass the `density`
Copy link
Member Author

Choose a reason for hiding this comment

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

@MDAnalysis/coredevs please look at discord for this one - in my opinion we probably should do a 2.4.2 release since this has effectively killed the DensityAnalysis code. However, how we do bugfix releases really should be discussed.

TLDR; I'm pushing for us to be able to do bugfixes from a separate feature branch than develop. That way PRs can keep rolling into develop without breaking semver if we need to do a bugfix release.

@IAlibay
Copy link
Member Author

IAlibay commented Dec 20, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@IAlibay
Copy link
Member Author

IAlibay commented Dec 21, 2022

So the consensus (from @orbeckst and @richardjgowers that spoke up on this), is that we'll do separate bugfix branches based off the release-2.4.x tags where we cherry-pick commits from develop (where possible) and do release from them (please correct me if I misinterpreted things!).

As such, I've moved the changelog entry here to be 2.4.2, with the aim that I'll just cherry-pick the squash-merged commit out of here and re-deploy (yay..).

@MDAnalysis/coredevs please review this as a priority - it's the main cause of current CI failures, and it's blocking the 2.4.x announcement (we can't announce it if it's failing as soon as it's released).

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

thank you, lgtm

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

LGTM

@hmacdope
Copy link
Member

@IAlibay only concern is the 32 bit build? Causing some failures on the Azure CI

Copy link
Member

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

CI failure looks like h5py not supporting NumPy 1.24.0 just yet on that platform.

32-bit Windows binaries seem to be getting dropped upstream a lot lately.

Releasing off of maintenance branches is what I do for SciPy and what Chuck does for NumPy, so +1 for that.


Fixes
* np.histogramdd calls in :class:`DensityAnalysis` now pass the `density`
argument rather than the NumPy 1.24 removed `density` (PR #3976)
Copy link
Member

Choose a reason for hiding this comment

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

you may have meant normed here at the end?

@IAlibay
Copy link
Member Author

IAlibay commented Dec 23, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@IAlibay
Copy link
Member Author

IAlibay commented Dec 23, 2022

we've had this particular h5py issue for some while (pre numpy 1.24 release), but the fact that there are more now might indicate that it's either related or it has accentuated the problem. I'm going to try switching the pipeline up a little bit to limit things for 32bit.

@IAlibay
Copy link
Member Author

IAlibay commented Dec 23, 2022

Only extra thing I've done is slightly change the azure pipelines to pin numpy for 32bit (hopefully this fixes it 🤞🏽 thanks @tylerjereddy !), so with two approvals I'm going to go ahead with the merge.

@IAlibay IAlibay merged commit 0c4f823 into develop Dec 23, 2022
@IAlibay IAlibay deleted the fix-density branch December 23, 2022 12:56
IAlibay added a commit that referenced this pull request Jan 4, 2023
* histogramdd normed argument replaced by density
* pin numpy for 32bit in azure pipelines
@IAlibay IAlibay mentioned this pull request Jan 4, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[upstream change] histogramdd normed has been removed
5 participants