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

Update analysis_slurm.py code for smoother analyses on Expanse #258

Conversation

bfhealy
Copy link
Collaborator

@bfhealy bfhealy commented Oct 5, 2023

This PR adjusts analysis_slurm.py to facilitate smoother analyses with Expanse:

  • The structure of directories and files created by analysis_slurm.py is modified, with the slurm script saved directly in the nmma directory.
  • More environment variables serve as wildcards for inputs specified by the user on SkyPortal (TMIN, TMAX, and DT).
  • The --outdir argument is adjusted to create a subdirectory labeled by each source's ID and analysis start time.
  • A test is added to tests/analysis.py to test slurm script generation.

Copy link
Member

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

LGTM

@mcoughlin
Copy link
Member

@bfhealy feel free to merge once tests pass

@sahiljhawar
Copy link
Member

sahiljhawar commented Oct 5, 2023

Tests failing!

analysis_slurm.main() is called without any arguments, even without slurm specific arguments!? Also I think args in test_analysis should be global so that test_analysis args’ can be accessed by test_analysis_slurm().

bfhealy and others added 4 commits October 6, 2023 10:02
using pytest fixtures instead of global. refactored code
args is updated in place, assigning it back to args returns None. Hopefully tests should pass now :')
@sahiljhawar
Copy link
Member

@mcoughlin I don't know about rebasing and dont want to mess up. Can you please merge this?

@mcoughlin mcoughlin merged commit 55d9f1c into nuclear-multimessenger-astronomy:main Oct 6, 2023
5 of 6 checks passed
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.

3 participants