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

resolve ariatools errors re directory behavior #169

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Conversation

jtencer
Copy link
Contributor

@jtencer jtencer commented Mar 20, 2024

Some of the recent changes broke regression tests on the aria side. These changes were necessary to resolve those issues.

@jtencer jtencer requested review from eparish1, pjb236 and fnrizzi March 20, 2024 19:44
@eparish1
Copy link
Contributor

@jtencer I'm fine with this; did you find the base directory useful for your stuff? Alternatively you could put the base directory into your prefix string. I got rid of it in the revision because I felt it was a little confusing to have the extra directory floating around and Patrick and Francesco mostly agreed, but I don't have strong feelings on this so am glad to keep it.

@jtencer
Copy link
Contributor Author

jtencer commented Mar 20, 2024

The problem I ran into was that it was hardwired to use the current working directory and I needed a way to pass in a location. If there's a different way to handle that I'd be open to it.

@eparish1
Copy link
Contributor

You can do, e.g., "run_directory_prefix = path_to_work/run_", and it will then put everything in "path_to_work". Do you like this, or do you prefer the base_dir concept?

@jtencer
Copy link
Contributor Author

jtencer commented Mar 21, 2024

I think I might prefer having only one argument but need it to support absolute paths. This change didn't introduce the base directory stuff. It just exposed it so that it could be something other than cwd.

I can push a change tomorrow morning that rips out all the base directory stuff.

@jtencer
Copy link
Contributor Author

jtencer commented Mar 21, 2024

I got rid of it in sampling. There are still references to base_directory in sampling_coupler_base.py and the greedy stuff but I'm guessing those will probably go away as we propagate the Model concept through.

Copy link
Contributor

@eparish1 eparish1 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jtencer jtencer merged commit 7602872 into develop Mar 21, 2024
6 checks passed
@fnrizzi fnrizzi deleted the sampling_updates branch May 15, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants