Closed bsipocz closed 2 months ago
Current status:
🔴 All have issues with the install/import cells mentioned above
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
Cell In[23], line 2
1 #proove we can do this for one object
----> 2 calc_instrflux(*paramlist[0][1:])
4 #same thing, different syntax
5 # calc_instrflux(paramlist[0][1], paramlist[0][2], paramlist[0][3], paramlist[0][4], paramlist[0][5], paramlist[0][6])
Cell In[21], line 54, in calc_instrflux(band, ra, dec, stype, ks_flux_aper2, img_pair, df) 49 subimage, bkgsubimage, x1, y1, subimage_wcs = cutout.extract_pair( 50 ra, dec, img_pair=img_pair, cutout_width=band.cutout_width, mosaic_pix_scale=band.mosaic_pix_scale 51 ) 53 # find nearby sources that are possible contaminants ---> 54 objsrc, nconfsrcs = find_nconfsources( 55 ra, dec, stype, ks_flux_aper2, x1, y1, band.cutout_width, subimage_wcs, df 56 ) 58 # estimate the background 59 skymean, skynoise = photometry.calc_background(bkgsubimage=bkgsubimage)
NameError: name 'find_nconfsources' is not defined
* 🔴 Light curve generator
- Errors out with:
AttributeError: module 'numpy' has no attribute 'msort'
* 🟠Light curve generate, large scale
- Works, but rendering is really ugly. Too many progressbar and other broken line wrappings, etc.
* 🟢 Light curve classifier
- Looks reasonable
* 🔴 AGN Zoo
- errors out with
AttributeError: np.Inf
was removed in the NumPy 2.0 release. Use np.inf
instead.
I cannot test this locally due to some install issues for healpy
Do you know where the healpy requirement is coming from? I searched the whole repo but see no mention of it. I had thought we already switched everything to use hpgeom instead. (And we switched because of the install issues with healpy.)
Do you know where the healpy requirement is coming from?
Yes, it's coming from nway
. The issue is that there are no wheels for older OSX with ARM chips, thus it tries to build it from the source and fails. I'm sure I could fix it by diving into the errors, but instead pinged the developers if they are willing to release more wheels
(Using CI to fix issues is I feel legit here, as we don't really need to support OSX and Windows for these notebooks; though this/similar issue will come up with lsdb/hipscat too when those pull in healpy as a dependency).
@bsipocz, how about using our fornax images in the CI? I did a little experiment to test that it can be done in this test-actions repo. It seems to work fine.
It pulls the images from the public aws ECR (in this case, tractor-develop
) and use docker to run code inside the image.
A few things may not run because we are outside the fornax system (e.g. restricted buckets etc.), but we can make the notebooks robust against that as part of their portability.
how about using our fornax images in the CI
Yes, I think this is a sensible idea, we may already have an issue about it. So far all the issues I run into while doing this PR are unrelated to the image (and e.g. the missing install of tractor; besides that I don't expect any image related problems).
Now this is placed on top of #318 and #321
The ML_AGNzoo notebook now runs into a file access issue: https://github.com/nasa-fornax/fornax-demo-notebooks/issues/322
Given #324, I'm adding that into the exclude list, too. So the scope of this PR will be scaled back to:
I will also propose to reshuffle the commits between this and #321, so all non-controversial cleanup commits can land in that one and be merged asap.
The latest commit of da1ccc154cccb191f42057f26e0db7b957d70608 should close #331
I cannot test this locally due to some install issues for
healpy
, so will use CI to iron out the issue. I'm sorry for the notification spams for those of you watching this repo.We need to handle dependencies sensibly.
These items are considered blockers:
Upstream issues, not blockers for this PR
Whishlist items, should not block merging