-
Notifications
You must be signed in to change notification settings - Fork 21
Feature: Split the array design save_layout into separate creation and save methods
#203
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
Feature: Split the array design save_layout into separate creation and save methods
#203
Conversation
nRiccobo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor details to address. Otherwise, it good to me. Much cleaner with a separate method for the layout_df.
| ValueError | ||
| Raised if ``folder`` is not one of "cables" or "plant". | ||
| """ | ||
| if folder not in ("cables", "plant"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be folder.lower()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if folder will be converted to lower case first, otherwise it would break later usage with ORBIT. If that's desired, then I can update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not the most pressing. It seems like the user would be familiar with the folder names prior to saving anyways...
| layout_df = self.create_layout_df() | ||
| data = [layout_df.columns] + layout_df.to_numpy().tolist() | ||
| print( | ||
| f"Saving custom array CSV to: <library_path>/cables/{save_name}.csv" # noqa: E501 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{folder}. If folder is set to plant, the print statement would tell me the wrong file location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good catch! I didn't really look at this part, but this is a great time to implement more useful logging.
This PR creates the method
ArraySystemDesign.create_layout_df()consisting of the layout data frame creation steps originally insave_layout(). To maintain backwards compatibility,save_layoutsimply calls the creation step, then proceeds with the save logic. By splitting this into separate methods the user can now obtain the layout data frame without having to save it to a file.