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

Doesn't quite work on osx #263

Closed
jeanconn opened this issue Dec 4, 2018 · 6 comments · Fixed by #302
Closed

Doesn't quite work on osx #263

jeanconn opened this issue Dec 4, 2018 · 6 comments · Fixed by #302

Comments

@jeanconn
Copy link
Contributor

jeanconn commented Dec 4, 2018

Looks like there are still a few tweaks that would be needed to run on OSX.

The starcheck wrapper is calling /bin/mktemp which doesn't exist. Maybe just mktemp would be OK.

The starcheck wrapper expects SKA_ARCH_OS to be defined. If we want to reduce Ska3 dependence to SKA, this should just be set to SKA.

The Dark Cal checker expects SKA_SHARE and SKA_DATA to be defined. These should probably be set to be their SKA equivalents or just reference the package area appropriately.

The initial conditions for the temperature model could probably just be set with maude to avoid the eng_archive access dependency.

@taldcroft
Copy link
Member

The starcheck wrapper is calling /bin/mktemp which doesn't exist. Maybe just mktemp would be OK.

👍

The starcheck wrapper expects SKA_ARCH_OS to be defined. If we want to reduce Ska3 dependence to SKA, this should just be set to SKA.

Yes, SKA_ARCH_OS is deprecated / broken in standalone Ska3 and should just be replaced.

The Dark Cal checker expects SKA_SHARE and SKA_DATA to be defined. These should probably be set to be their SKA equivalents or just reference the package area appropriately.

Likewise those are deprecated / broken and should be replaced with SKA/data,share without thinking twice.

The initial conditions for the temperature model could probably just be set with maude to avoid the eng_archive access dependency.

Use fetch.data_source.set('cxc', 'maude') to fall back to MAUDE if necessary.

@jeanconn
Copy link
Contributor Author

jeanconn commented Dec 4, 2018

Likewise those are deprecated / broken and should be replaced with SKA/data,share without thinking twice.

Well, I am thinking twice when I have a tested 12.1 release candidate with built conda packages that I'd like to promote. Plan was to come back for OSX fixes in the future (I was thinking the proseco release) which is why I captured my thoughts on what looked like the initial issues with running the current starcheck 12.1 package.

@jeanconn
Copy link
Contributor Author

jeanconn commented Dec 4, 2018

It also looks like the SKA_SHARE use in the dark cal checker is setting a variable that is never used again, so overall it could just use some trimming and more cleanup. And I think that the data dir used by the dark cal checker should be set to the package area from the calling code in starcheck.pl instead of SKA/data .

And then there are a bunch of places in the starcheck code where it is set to do an "if SKA not defined set to /proj/sot/ska" so those should all be replaced with code that just errors out instead.

@taldcroft
Copy link
Member

Plan was to come back for OSX fixes in the future

Of course. Just meant that doing these changes in the future shouldn't require any discussion, just do it.

@jeanconn
Copy link
Contributor Author

jeanconn commented Dec 4, 2018

Sure. I didn't think that opening an issue so I'd remember to look at this would create a discussion.

@jeanconn
Copy link
Contributor Author

jeanconn commented Jan 8, 2019

Regarding "Use fetch.data_source.set('cxc', 'maude') to fall back to MAUDE if necessary.", I've forgotten, do I need to calculate pitch from attitudes and times if we use maude data?

@jeanconn jeanconn added this to the 13.1 milestone Jan 17, 2019
@jeanconn jeanconn modified the milestones: 13.1, 13.2 Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants