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 master with develop #185

Merged
merged 30 commits into from
Jan 26, 2022
Merged

update master with develop #185

merged 30 commits into from
Jan 26, 2022

Conversation

orbeckst
Copy link
Member

The develop branch contains important contributions. At the same time, master contains commits that develop does not have.

This PR shows what is different and asks for clarification where changes for the User Guide should be submitted.

@lilyminium
Copy link
Member

lilyminium commented Jan 21, 2022

I think they got minorly out of sync around 1.x, and then majorly around 2.x, when I forgot to backport/forwardport some fixes. Merging develop into master has the same headachy conflicts that the main repo dealt with around 2.x when 1.0+ was still being added onto, because the 0.20.1 and 1.x versions will still getting new content (or 0.20.1 and 1.x-specific notes) here, while develop was on notes on 1.x or 2.x respectively. I'll detangle them now and clean up the commit history.

master here is supposed to contain the user guide for master MDAnalysis, and develop for develop MDAnalysis.

Edit: #119 is related but possibly out of date now.

@orbeckst
Copy link
Member Author

If master is supposed to track released versions of MDA then there are some things that should go into master right away, such as the H-bonds pages and the fixes for displaying the PCA and HOLE2 notebooks (closed issues #175 #176) and issue #183 (PR #184).

Would it be sensible to switch to a model where there's only main, which corresponds to ongoing MDA develop, and the released versions are branched off from main? One might then still need to backport commits onto those release branches but hopefully that doesn't happen to often but you also have a constantly improving branch and it's less likely that something is forgotten.

At the moment, issues are only auto-closed when something is merged into master and not develop, i.e., when something is fixed in the current stable docs. Changing to the other model would close issues when the fix is in development docs. I am not 100% sure what the better approach is.

I also don't know how much the deployment magic under the hood relies on the current branching model. It might just be too much work to change it.

@orbeckst orbeckst marked this pull request as draft January 21, 2022 22:04
@orbeckst
Copy link
Member Author

I marked as "draft" so that this doesn't get merged unwittingly. Thanks for the explanation #185 (comment).

@lilyminium
Copy link
Member

I'd prefer the branch names to mirror the MDAnalysis branch names for least confusion (has MDA switched to main?). But you're right that making release-specific branches would be ideal.

Ideally develop should be the "main" branch here, that new updates go into. Then 1.x and 0.x-relevant fixes can be ported one-way from develop into those branches. Currently everything's a mess, so I'm working on #186 to port the master updates into develop, so develop can be merged into master for 2.0; there haven't been any updates for 2.1 so far.

I also don't know how much the deployment magic under the hood relies on the current branching model. It might just be too much work to change it.

This isn't too hard! Just changing the entry in environment.yml will make docs render for that particular version, although we'll need to update CI to run the merge for all release-* branches too.

@orbeckst
Copy link
Member Author

So then I should be pushing for MDA to go to main and then the User Guide will just follow? ;-)

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

mieczyslaw and others added 14 commits January 22, 2022 11:24
* Fixes issue #100
* adds examples of selecting residues from each segment, or atoms from each residue
* Fixes #93
* Adds tutorials on various ways to do analysis in parallel
- Changed 'selection' keyword to 'select'
- Added radius_cut_q and explained results more
- Added background section
- Added bit more intro to readme
- added `match_atoms` keyword
- clarified some of the `in_memory`/filename reasons
Changes:
- Added links to API docs for each class/function in each tutorial
- Updated links in Quickstart guide
- Fixed link to alignment example notebook
Add Binder badge
@lilyminium
Copy link
Member

So then I should be pushing for MDA to go to main and then the User Guide will just follow? ;-)

Yup, BOGO!

Huge apologies, I made a bit of a mess with the commit history here. I thought cherry-picking commits would make the merge easier... it didn't. I'd like to propose:

  • merging develop into master
  • Re-committing to master the master-specific settings (pointing to the latest release of MDAnalysis)
  • Either renaming master to 2.0 or leaving as is until MDAnalysis does something about the name
  • Forking off a release-1.1.1 branch at e88eeef (the last branch before merging some 2.x specific updates)
  • Cherry-picking b2c20d7 and ecc4d2f + 6e223d0 into release-1.1.1 (general fixes)
  • Make develop the new main branch, backport any general updates to master and/or release-1.1.1

Thoughts?

I could remove the cherry-picked master commits from here, but I don't really know if messing with the history is a wise idea. :/

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

LGTM.

I went through all these changes when I was looking at merge conflicts etc.

For all notebooks listed, there are minor differences in the Python version used, object ids in memory, kernel name, metadata, etc., the things that differ the notebook is re-executed. I list more important differences below:

  • The giant .ipynb diffs for alignment notebooks are from the nglview widgets and minor differences in Python version
  • custom_parallel_analysis has actual new additions for 2.0, as picking wasn't in 1.1
  • hole.ipynb fixes some embarrassing merge conflicts left inside current master.
  • pca.ipynb incorporates @orbeckst's .flatten() fix
  • hydrogen_bonds is totally new
  • helanal is totally new

Other files:

  • Pickling is new in 2.x
  • environment.yml points to develop MDAnalysis -- this should be fixed in master

@lilyminium lilyminium marked this pull request as ready for review January 22, 2022 07:23
@lilyminium lilyminium merged commit bda0a06 into master Jan 26, 2022
@lilyminium lilyminium deleted the develop branch January 26, 2022 12:05
@lilyminium lilyminium restored the develop branch January 26, 2022 12:05
@orbeckst
Copy link
Member Author

sorry @lilyminium I left you alone with this whole thing. Many thanks for fixing and merging it, a great step forward for the UG.

I put the master/develop -> main discussion on the agenda for the meeting today.

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.

6 participants