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

Bug/fix srctool output #1269

Open
wants to merge 7 commits into
base: feat/integrateMultiMiss
Choose a base branch
from

Conversation

HallJoseph
Copy link
Collaborator

@HallJoseph HallJoseph commented Nov 21, 2024

Ok we have figured out what is happening - is to do with the new version of srctool in eSASSDR1. There is a new parameter “write_insts”, by default this will be “0 1 2 3 4 5 6 7 8 9", so when you run srctool for individual instruments you will currently output: 020_SourceSpec.fits, 220_SourceSpec.fits, 820_SourceSpec.fits. Therefore when the next command is run for the next instrument it will try to output files called 020_SourceSpec.fits and 820_SourceSpec.fits, which already exist. So it errors because it can’t write out the file.

Have added commands to remove these extra files.

Have also added checking of eSASS version in utils fixed a bug where an error was generated if ciao was not initialised.

@HallJoseph HallJoseph self-assigned this Nov 21, 2024
Copy link
Owner

@DavidT3 DavidT3 left a comment

Choose a reason for hiding this comment

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

Nice one, thanks very much for fixing that XGA-eROSITA bug! And spotting the annoying Chandra behaviour we introduced.

xga/generate/esass/spec.py Show resolved Hide resolved
xga/utils.py Show resolved Hide resolved
xga/utils.py Show resolved Hide resolved
xga/utils.py Show resolved Hide resolved
@@ -677,8 +684,9 @@ def check_telescope_choices(telescope: Union[str, List[str]]) -> List[str]:
ciao_err = ciao_err.decode("UTF-8")

if "ciaover: command not found" in ciao_err:
warn("No CIAO installation detected on system, "
"as such all functions in xga.generate.ciao will not work.", stacklevel=2)
split_out = ''
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for spotting this problem - I missed it when I did code review for that PR. Rather than defining a blank split out, please could you check if CIAO_VERSION is None further down when it gets to the CALDB version check that was failing - I'll put another comment where I mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't work when I try to implement it, if I don't define split_out the code falls over at the if statement later because split_out is not defined anywhere. Alternatively, setting split_out = [en.strip(' ') for en in ciao_out.split('\n')] also doesn't work because ciao_out is empty if ciao is not installed/initialised. Using ciao_err instead of ciao_out throws an error too because in that case split_out has less than 6 elements so the indexing in the if statement stops working.

I think we found this was happening when we wrote this which is why we defined the blank split_out here.

@@ -694,7 +702,10 @@ def check_telescope_choices(telescope: Union[str, List[str]]) -> List[str]:
# This checks for an installation of Ciao
CALDB_AVAIL = False

if 'not installed' in split_out[5].lower():
if split_out == '':
Copy link
Owner

Choose a reason for hiding this comment

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

So rather than this if addition and extra warning, please change the

elif 'not installed' in split_out[5].lower():

to

if CIAO_VERSION is not None and 'not installed' in split_out[5].lower():

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See reply to comment above (line 687)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants