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

Fix nbval testing failures due to new 0.10 release #251

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Fix nbval testing failures due to new 0.10 release #251

merged 1 commit into from
Jun 14, 2023

Conversation

pgbarletta
Copy link
Contributor

From #248 and #249

One small drawback is that for now I can't silence this ugly plots:
ugly_plot

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@pgbarletta
Copy link
Contributor Author

Should I close this?

@IAlibay
Copy link
Member

IAlibay commented Jun 9, 2023

Should I close this?

I have no context for this fix, is it still a necessary change in your opinion?

@pgbarletta
Copy link
Contributor Author

Well, this was done to fix #248 and #249, and back then it worked (IDK why it's failing right now).
But then it seems that you pinned nbval on #250 and carried on.
I could rebase this branch, make everything pass again and unpin nbval if you want.

The nbval bug that caused this is still there by the way.

@IAlibay
Copy link
Member

IAlibay commented Jun 9, 2023

Well, this was done to fix #248 and #249, and back then it worked (IDK why it's failing right now). But then it seems that you pinned nbval on #250 and carried on. I could rebase this branch, make everything pass again and unpin nbval if you want.

The nbval bug that caused this is still there by the way.

Ok, yeah it looks like this fix is still necessary. If you have the capacity to work out why things are failing now then that'd be great. Please go ahead and ping the whole coredevs group when you need a review (I don't spend a lot of time looking at this repo unfortunately).

(#250 was just a temporary fix so I could get a release out)

@pgbarletta
Copy link
Contributor Author

@MDAnalysis/coredevs Done!

No big changes here. Just sanitizing notebooks* so nbval tests pass without the need of pinning it to an older version.

*: that is, adding regex expressions so outputs that necessarily vary from execution to execution (eg: data structures' memory locations) don't make nbval fail spuriously.

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.

Thanks @pgbarletta for getting on this. Looks good, just a suggestion: ax = ps.plot(...) should still show the plot, I think, but with a much smaller output.
Screenshot 2023-06-10 at 3 41 35 pm

@pgbarletta pgbarletta requested a review from lilyminium June 10, 2023 07:33
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!

Sorry for all the conflicts...

@pgbarletta
Copy link
Contributor Author

LGTM!

Sorry for all the conflicts...

Yeah, do you know why they showed up all of a sudden?

@lilyminium
Copy link
Member

I'm guessing because I merged #256 -- the widget states might be conflicting?

@pgbarletta
Copy link
Contributor Author

ah, right, it's gotta be that.

@pgbarletta pgbarletta requested a review from IAlibay June 12, 2023 10:43
@IAlibay
Copy link
Member

IAlibay commented Jun 12, 2023

Just saw the review request - apologies @pgbarletta I'm kinda checked out of userguide things for the time being. I'm leaving it up to @lilyminium to take this towards a merge.

@lilyminium
Copy link
Member

@pgbarletta are you up for fixing the merge conflicts? Otherwise this PR is good to go :)

@pgbarletta
Copy link
Contributor Author

Done! Removed all the widget attributes.

@lilyminium
Copy link
Member

Thanks so much for this @pgbarletta!

@lilyminium lilyminium merged commit 09a4d96 into MDAnalysis:develop Jun 14, 2023
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.

3 participants