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.

Sign up for GitHub

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

Jump to bottom

Store configuration parameters #891

Merged
merged 8 commits into from
Oct 24, 2024
Merged

Store configuration parameters #891

merged 8 commits into from
Oct 24, 2024

Conversation

gonzaponte
Copy link
Collaborator

@gonzaponte gonzaponte commented Sep 20, 2024 •
edited
Loading

This PR stores the city configuration in the output file for bookkeeping purposes. It also propagates the configuration from previous cities further down the chain.

Closes #837

@gonzaponte
Copy link
Collaborator Author

This PR needs some others merged first.

@gonzaponte gonzaponte marked this pull request as draft September 21, 2024 18:33
bpalmeiro
bpalmeiro reviewed Oct 3, 2024
View reviewed changes
Copy link
Member

@bpalmeiro bpalmeiro left a comment

Choose a reason for hiding this comment

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

Long-awaited enhancement finally coming to place! Nice and simple changes. Just a few comments/curiosities

invisible_cities/cities/components.py
.to_frame()
.reset_index()
.rename(columns={"index": "variable", 0: "value"}))
df_writer(file, df, "config", city_name, f"configuration for {city_name}", str_col_length=300)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just out of curiosity, why the 300 and why hardcoded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is just long enough for our use case. There is no better reason than that, and there is no "right" value that can guarantee all potential values will fit. It's also the same value used in nexus to store the configuration, so it seemed appropriate.

invisible_cities/cities/components.py Outdated Show resolved Hide resolved
invisible_cities/cities/cities_test.py Show resolved Hide resolved
invisible_cities/cities/cities_test.py Show resolved Hide resolved
@gonzaponte gonzaponte marked this pull request as ready for review October 10, 2024 11:10
@gonzaponte
Copy link
Collaborator Author

I really like this test and just wondering if it is easy to expand it for testing the copy

I've added a test that ensures that the configuration is propagated for two consecutive cities. Doing it in general is a bit

Why a silent return? Isn't it better any sort of warning?
This behavior's worth testing anyway.

I've added a warning and the corresponding test

bpalmeiro
bpalmeiro approved these changes Oct 21, 2024
View reviewed changes
Copy link
Member

@bpalmeiro bpalmeiro left a comment

Choose a reason for hiding this comment

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

Just to repeat myself, long-awaited enhancement finally coming to place! All the suggestions were taken care of and the code is neat and clear. Great job!

@jwaiton jwaiton merged commit 188dcde into next-exp:master Oct 24, 2024
1 check passed
@gonzaponte gonzaponte deleted the store-config branch October 25, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpalmeiro bpalmeiro bpalmeiro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store cities' arguments in the output file
3 participants
@gonzaponte @bpalmeiro @jwaiton