Yamlification of parastell #56
Conversation
There was a problem hiding this comment.
Thanks for this addition @FusionSandwich - I have a few questions/suggestions.
| build = { | ||
| 'phi_list': phi_list, | ||
| 'theta_list': theta_list, | ||
| 'wall_s': wall_s, | ||
| 'radial_build': radial_build['components'], | ||
| 'components': {} | ||
| } |
There was a problem hiding this comment.
Can't you just define this as config['radial_build'] and then process the details into an NumPy array?
There was a problem hiding this comment.
It seemed like it made more sense to have similar parameters grouped together in the yaml file. Even though they are apart of different inputs to parastell. I wasn't entirely sure if we'd be able to group the inputs for parastell together. And I didn't want to assign values to build that wouldn't be called elsewhere. But I can separate out variables from the radial_build in the yaml file like wall_s and repeated.
There was a problem hiding this comment.
Then I can just set build as config['radial_build'] without the issues.
There was a problem hiding this comment.
I think this may be worth a conversation as a group to balance the user experience in the YAML file vs the code benefits for the developers (see #52)
|
|
||
| # Convert thickness_matrix in 'radial_build' components to numpy arrays | ||
| for component, details in radial_build['components'].items(): | ||
| if 'thickness_matrix' in details and isinstance(details['thickness_matrix'], list): |
There was a problem hiding this comment.
What if it's not a list? is in an error?
There was a problem hiding this comment.
Yeah that was my experience, but I'll run some more tests and check it out again.
There was a problem hiding this comment.
This may be a question for @connoramoreno , but one wanted a uniform thickness blanket, how would it be defined in the current struture? Would it still be a theta/phi grid with very few points (2x2)? Or could it be a scalar for each thickness? Do we have to handle cases like that?
There was a problem hiding this comment.
I'll ask Conner about it before the Thursday meeting.
There was a problem hiding this comment.
Sorry, the notification for this got lost in my email inbox. In the current implementation, components of uniform thickness are still defined on a grid but all elements have equal value. We could choose to handle input for uniform thickness components differently, of course.
There was a problem hiding this comment.
We should probably issue an error if this data is NOT a list, then.
|
Please rebase this to capture updates to .gitignore. I'd like to merge it ASAP |
|
Something's fishy in this PR - there are too many files included that are not different from the target branch. We may need to do a little git repair... |
|
Yeah haha I was having some weird issues rebasing it. Even Edgar was surprised with how it was acting.
…________________________________
From: Paul Wilson ***@***.***>
Sent: Thursday, March 14, 2024 8:14:08 a.m.
To: svalinn/parastell ***@***.***>
Cc: Joshua Taylor Dean SMANDYCH ***@***.***>; Mention ***@***.***>
Subject: Re: [svalinn/parastell] Initial Commit Yaml Example Script (PR #56)
Something's fishy in this PR - there are too many files included that are not different from the target branch. We may need to do a little git repair...
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/svalinn/parastell/pull/56*issuecomment-1997435006__;Iw!!Mak6IKo!PoOdhBMqhaifnmj38UzxUqzGxV0qfX4ZotMK4MFbqb83oJrPxFRVM9wtyIbYlliwkTEfZrh-kSkkTFLcTtjfvZuf6Q$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/BCUBXTLEEGV5VGUBHHOKAN3YYGPBXAVCNFSM6AAAAABEIEOOJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJXGQZTKMBQGY__;!!Mak6IKo!PoOdhBMqhaifnmj38UzxUqzGxV0qfX4ZotMK4MFbqb83oJrPxFRVM9wtyIbYlliwkTEfZrh-kSkkTFLcTtjSvFfPIQ$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I don't know how it got to this state, but I was able to rebase it cleanly. It would be great to sort this out today, even if you need the help of some other folks to navigate the git. |
|
Yeah I'm going to put this issue and the conversation about the parastell yaml file in the hacker within agenda.
…________________________________
From: Paul Wilson ***@***.***>
Sent: Friday, March 15, 2024 8:41:10 AM
To: svalinn/parastell ***@***.***>
Cc: Joshua Taylor Dean SMANDYCH ***@***.***>; Mention ***@***.***>
Subject: Re: [svalinn/parastell] Initial Commit Yaml Example Script (PR #56)
I don't know how it got to this state, but I was able to rebase it cleanly. It would be great to sort this out today, even if you need the help of some other folks to navigate the git.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/svalinn/parastell/pull/56*issuecomment-1999690855__;Iw!!Mak6IKo!PKIW-M6Nz7esboBq2z0ezFdStLXol6XK_pTX3aHCafthSj6To7zaPYBKp3F03b3LLbt-VqK0iAQEjdBMRkfe_PZJmw$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/BCUBXTLJF6X7DGM4HV47LVDYYL27NAVCNFSM6AAAAABEIEOOJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJZGY4TAOBVGU__;!!Mak6IKo!PKIW-M6Nz7esboBq2z0ezFdStLXol6XK_pTX3aHCafthSj6To7zaPYBKp3F03b3LLbt-VqK0iAQEjdBMRkfWxcv41g$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
FusionSandwich
force-pushed
the
yaml_input
branch
from
c00c972 to
8574711
Compare
|
Did we do what you expected with it? |
There was a problem hiding this comment.
@FusionSandwich when you get a chance, like we discussed "Yamlification of parastell" might also be a good name for this PR (Instead of "Initial Commit Yaml Example Script").
I think our rebase and force push likely addressed @gonuke's concern of extra files getting merged that this PR shouldn't have changed.
FusionSandwich
changed the title
Included Yaml input file and new python execution file. Current issue with getting to write to a yaml output file. Will likely be updated depending on how strengths output changes.
Closes #45