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

Fix Psi4 load #65

Closed
wants to merge 1 commit into from
Closed

Fix Psi4 load #65

wants to merge 1 commit into from

Conversation

vaishakp
Copy link
Collaborator

@vaishakp vaishakp commented Dec 19, 2024

  1. RIT Psi4 load
    Multiple issues were found with existing Psi4 load. Was taking time to fix all.
    The core of the load is now delegated to waveformtools. Tested and working.
  2. Minor improvements
  3. Lint

FFI coming up next

remove tgz v2 copy
Copy link
Contributor

@prayush prayush left a comment

Choose a reason for hiding this comment

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

If there is any error in the current loading functionality for Psi4, please fix that in this package. Lets not offload it to another package, with the only acceptable exception being packages being maintained by NR groups themselves (i.e. sxs and mayawaves).

Comment on lines +220 to +222
wftools_modes_array = load_RIT_Psi4_data_from_disk(
data_file_path=file_path, output_modes_array=True, resam_type="finest"
)
Copy link
Contributor

@prayush prayush Dec 20, 2024

Choose a reason for hiding this comment

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

Lets not delegate this IO task to another package. Offloading minor math operations is fine, but this functionality is central to the nr-catalog-tools package to begin with.

@prayush prayush deleted the branch gwnrtools:updates December 20, 2024 11:49
@prayush prayush closed this Dec 20, 2024
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