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 eta-v matplotlib plot #340

Merged
merged 3 commits into from
Mar 6, 2022
Merged

Fix eta-v matplotlib plot #340

merged 3 commits into from
Mar 6, 2022

Conversation

ddobie
Copy link
Collaborator

@ddobie ddobie commented Mar 6, 2022

Fix #336.

@pep8speaks
Copy link

pep8speaks commented Mar 6, 2022

Hello @ddobie! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-06 11:26:28 UTC

@ddobie ddobie requested a review from ajstewart March 6, 2022 11:30
@ddobie ddobie marked this pull request as ready for review March 6, 2022 11:30
Copy link
Collaborator

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

That was easy, was it something obvious that this was the solution? Has the behaviour of position changed in the subplot declaration?

Need to make sure that notebooks are refreshed before a 3.0 release (I guess there needs to be a general docs go-over as well for that massive PR that was merged).

@ddobie
Copy link
Collaborator Author

ddobie commented Mar 6, 2022

That was easy, was it something obvious that this was the solution?

I ran into the same issue when I was re-doing the eta-v plot for the latest GW190814 paper and fixed it on my own branch but for some reason forgot to actually push it. Speaking of which, I also wrote all the code to fix #275, although I think that branch was only on the hub-dev nimbus instance which is now gone.

Has the behaviour of position changed in the subplot declaration?

I never figured out the cause, just the solution. As far as I can tell that behaviour hasn't changed, so my assumption is that something further down the function is overwriting the initial position declaration.

Need to make sure that notebooks are refreshed before a 3.0 release (I guess there needs to be a general docs go-over as well for that massive PR that was merged).

Agreed. I'm pretty sure the docs are actually up to scratch as I was quite thorough when doing that massive PR, but it warrants checking anyway.

Funnily enough I actually noticed an error in the docs this week when using them to provide some example code to a colleague. In https://www.vast-survey.org/vast-tools/notebook-examples/vast-pipeline-example/#performing-transient-and-variable-analysis you mention using df.filter but I believe (based on my knowledge of pandas - I haven't actually checked the vast-tools code) it's actually df.query. I'll try and do a thorough docs check before v3.0.0.

@ddobie ddobie merged commit c704aaf into dev Mar 6, 2022
@ajstewart
Copy link
Collaborator

Funnily enough I actually noticed an error in the docs this week when using them to provide some example code to a colleague. In https://www.vast-survey.org/vast-tools/notebook-examples/vast-pipeline-example/#performing-transient-and-variable-analysis you mention using df.filter but I believe (based on my knowledge of pandas - I haven't actually checked the vast-tools code) it's actually df.query. I'll try and do a thorough docs check before v3.0.0.

Yep pretty sure that should be query! 😅

@ajstewart ajstewart deleted the iss336 branch March 6, 2022 11:57
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.

matplotlib eta-V plot layout sizing issue
3 participants