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

Force plotAssemblyTypes to always use instantiated assemblies, and al… #2030

Merged
merged 11 commits into from
Jan 2, 2025

Conversation

keckler
Copy link
Member

@keckler keckler commented Dec 10, 2024

This is meant to improve #1998 by allowing the specification of hot versus cold heights in the plots. Additionally, a couple clean ups are implemented:

  1. Change plotAssemblyTypes() so that it always operates based on instantiated assemblies. Allowing users to pass in blueprints was making things really messy, and provided absolutely no benefit because blueprints already contain Assembly objects in the first place through blueprints.assemblies.values().
  2. The point above means that the plotAssemblyTypes() method signature is changed. That signature is/was pretty ridiculous anyways, so I feel justified in changing it. I updated all calls to that method that I could find.

I would not expect that there are many downstream calls to this plotAssemblyTypes() method. Checking all the projects that I have access to, I found it used in exactly one place (one of my own internal repos).

@keckler
Copy link
Member Author

keckler commented Dec 10, 2024

I am relatively sure that this is working now. I tested it with a simply dummy case, however I am unable to test it with my big model due to some dependency conflicts. I think it is ready for your eyes @john-science

@keckler keckler marked this pull request as ready for review December 10, 2024 17:04
@john-science john-science added the bug Something is wrong: Highest Priority label Dec 18, 2024
@john-science john-science self-requested a review December 18, 2024 16:46
@john-science
Copy link
Member

Love it!

  1. Can we put something under the 0.5.1 release notes for "bug fixes"?
  2. Does this close a ticket we can link to it?
  3. Should we close my previous PR on the matter?

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

Love it

@keckler
Copy link
Member Author

keckler commented Jan 2, 2025

Love it!

  1. Can we put something under the 0.5.1 release notes for "bug fixes"?
  2. Does this close a ticket we can link to it?
  3. Should we close my previous PR on the matter?
  1. I added the release note
  2. See point (3) below
  3. My intention was to merge these changes into your branch (PR Fixing a couple of plots to use initial block height #1998 ). You'll note that I currently have this PR pointed that way and was working off of your branch when I developed this code. If we do that, then I think your PR is already linked to the appropriate ticket (Clashing prints on blueprints pictures #1158 ).

Let me know what you think of that plan.

@john-science
Copy link
Member

3. You'll note that I currently have this PR pointed that way and was working off of your branch

Ah! No, I didn't notice that. I just assumed this was pointed at main.

Thanks!

@john-science john-science merged commit 0e1950e into fix_plots_block_heights Jan 2, 2025
17 checks passed
@john-science john-science deleted the fix_plots_block_heights_again branch January 2, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants