Skip to content

Conversation

@donald-e-boyce
Copy link
Collaborator

The intention of this pull request is to make the output file placement in find-orientations and fit-grains cleaner and simpler while cleaning up computational code that has hardwired file names and file handling that would be better done elsewhere, i. e. the config module.

Roughly, this is what has been done:

  • put output files in the analysis_dir instead of in the working_dir with long names based on the analysis_name
  • allow for slashes in analysis_name and automatically create subdirectories as needed for the analysis_dir
    • this is particularly useful when the config file has multiple (YAML) documents
  • move path manipulations and hardwired file names from computational modules to the config module
  • use pathlib for handling file and directory names
  • update docstrings where useful (for affected modules/functions)

You activate the new file placement with the new_file_placement keyword in the root level of the config file, e.g.

analysis_name: ruby/scan-0  

# working_dir:
new_file_placement: on

If the new_file_placement is False (the default), then nothing is changed; the old file placement is used.

Originally, this was intended mainly for find-orientations, but fit-grains is slightly affected since it looks for a file written by find-orientations, i.e. the accepted orientations.

Running this on the multiruby and single_GE NIST_Ruby examples, I get the exact same results using the new or the old file placement.

@pep8speaks
Copy link

pep8speaks commented Dec 3, 2024

Hello @donald-e-boyce! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 127:59: E225 missing whitespace around operator

Comment last updated at 2024-12-11 16:24:01 UTC

Copy link
Collaborator

@psavery psavery left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a huge improvement! It is much better to contain the logic for files/directories within the config, rather than within the find-orientations and fit-grains functions. I left some comments - they are only minor things.

Can you make sure that the tests pass? It looks like some of the tests are failing, because apparently comparing Path objects with strings always fails (for example, cfg.working_dir == '/some/path' always fails now, because Path('/some/path') != '/some/path').

I think you just need to get the tests to use a Path object now in the comparison, or convert the Path object to str, whichever you prefer.

There are apparently some packaging issues too that I will look over.

@psavery
Copy link
Collaborator

psavery commented Dec 11, 2024

Thanks Don! The changes look good so far! Now we just need to fix the tests (as mentioned here).

Copy link
Collaborator

@psavery psavery 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! I fixed the packaging issues in master (so they won't be an issue after merging).

@donald-e-boyce donald-e-boyce merged commit 3bff50c into HEXRD:master Dec 12, 2024
4 of 6 checks passed
@donald-e-boyce donald-e-boyce mentioned this pull request Dec 30, 2024
@donald-e-boyce donald-e-boyce deleted the find-orientations-files branch September 20, 2025 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants