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

Physics package fixes #1754

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Physics package fixes #1754

merged 5 commits into from
Nov 13, 2024

Conversation

saransh13
Copy link
Member

The thicknesses of each layer kept increasing upon repeated selection of pinhole correction. The problem was due too using a dictionary by reference. Fixed by making a copy and editing it instead of original dictionary.

@pep8speaks
Copy link

pep8speaks commented Nov 8, 2024

Hello @saransh13! 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 2024-11-13 13:42:58 UTC

@saransh13 saransh13 added bug Something isn't working llnl labels Nov 11, 2024
Comment on lines 1157 to 1158
# self.axis.xaxis.set_major_locator(PolarXAxisTickLocator(self))
# axis.xaxis.set_major_locator(PolarXAxisTickLocator(self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I think we were using these before so that when you set the x-axis to use Q instead, tick markers show up at 1, 2, 3, 4, .... Can you verify that still happens with the above changes? If not, we might need to fix that before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, looks like switching to Q messes up the xticklabels. We'd have to redo this step when the axis is updated to use Q.

Screenshot 2024-11-11 at 2 57 17 PM Screenshot 2024-11-11 at 2 57 32 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

So actually, those PolarXAxisTickLocator classes inherit AutoLocator. I think that in the code above, where you have yaxis.set_major_locator(AutoLocator()), we might be able to just change that AutoLocator to PolarXAxisTickLocator.

However, we probably also need to change the minor locator, too.

Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

I removed the polar view style changes. If this looks good to you, feel free to merge.

@saransh13 saransh13 merged commit e36ac12 into master Nov 13, 2024
9 checks passed
@saransh13 saransh13 deleted the physics-package-fixes branch November 13, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working llnl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants