Skip to content

Fix file reading bug in #14#16

Open
ced4rtree wants to merge 5 commits into
cmelab:mainfrom
ced4rtree:energy_pair_fix
Open

Fix file reading bug in #14#16
ced4rtree wants to merge 5 commits into
cmelab:mainfrom
ced4rtree:energy_pair_fix

Conversation

@ced4rtree
Copy link
Copy Markdown
Contributor

@ced4rtree ced4rtree commented Jun 1, 2026

See #14

I forgot to parameterize the check_pair_energy function in dpd_utils.py but failed to catch it since the function was reading from an old log file while I was testing. I added a test in this PR to check functionality of both creation & reading of the log files produced when a log file is specified.

ced4rtree added 3 commits June 1, 2026 13:49
fortunately didn't break any existing code, but broke new code that
specifies a log file other than log.txt
@ced4rtree
Copy link
Copy Markdown
Contributor Author

After checking this again, I'm wondering if log_file_name should really have a default value. It's only used internally and having a default value for file reading seems like an easy footgun.

@erjank
Copy link
Copy Markdown
Contributor

erjank commented Jun 2, 2026

I'm not clear on the proposed path forward. On one hand, "log.txt" is a great default filename, but if the worry is that we'll overwrite something that we'll need, then creating unique identifiers for each run and then cleaning them up (or not) seems like the sensible choice.

@ced4rtree
Copy link
Copy Markdown
Contributor Author

The worry in my comment was about reading from the wrong data. Defaulting to overwriting files with a default filename is the status quo in FlowerMD, so I'm fine with keeping that (but not opposed to changing if that's desired). What I'm worried about in this case is if somebody has a log.txt in the working directory and calls check_pair_energy but forgets to supply the file name of the file they want to read from, it would lead to reading from the wrong log.

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