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

add guide star catalog flag #141

Merged
merged 1 commit into from
Mar 20, 2025
Merged

add guide star catalog flag #141

merged 1 commit into from
Mar 20, 2025

Conversation

kiyoyabe
Copy link
Contributor

No description provided.

@kiyoyabe kiyoyabe requested review from wtgee and alefur March 19, 2025 02:04
Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

Minor changes except the auto switch, which should be done before merging.

Comment on lines 251 to 252
ASTROMETRIC: Astrometric excess noise is small (astrometric_excess_noise<1.0).
ASTROMETRIC_SIG: Astrometric excess noise is significant (astrometric_excess_noise_sig<2.0).
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel like these names make sense but we can keep them. It should be a high and low limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you are right. We will not use this info this time, but I'll fix that.

A dimensionless measure (𝐷) of the significance of the calculated astrometric_excess_noise (𝜖𝑖). A value 𝐷>2 indicates that the given 𝜖𝑖 is probably significant.

https://gea.esac.esa.int/archive/documentation/GDR3/Gaia_archive/chap_datamodel/sec_dm_main_source_catalogue/ssec_dm_gaia_source.html

PARA_SIG = auto()
ASTROMETRIC = auto()
ASTROMETRIC_SIG = auto()
RUWE = auto()
Copy link
Member

Choose a reason for hiding this comment

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

Should this be BINARY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say NON_BINARY.

@@ -48,6 +49,8 @@ class GuideStars:
guideStarCatId : `int`
The identifier for the catalogue from which the guide stars
were taken.
flag : `numpy.ndarray' of `int`
The flag infromation for the guidestar catalog.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The flag infromation for the guidestar catalog.
The flag information for the guidestar catalog.

PHOTO_SIG: Photometric measurement is significant (SNR>5).
GALAXY: Is a galaxy.
"""
GAIA = auto()
Copy link
Member

Choose a reason for hiding this comment

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

These should probably not use auto:

Suggested change
GAIA = auto()
GAIA = 0x0001

@alefur
Copy link
Contributor

alefur commented Mar 19, 2025

Flake8 is not happy.

Failed test output:
./python/pfs/datamodel/guideStars.py:3:1: F401 'enum.auto' imported but unused
./python/pfs/datamodel/guideStars.py:191:63: W291 trailing whitespace
./python/pfs/datamodel/guideStars.py:238:1: E302 expected 2 blank lines, found 1
./python/pfs/datamodel/guideStars.py:271:1: E302 expected 2 blank lines, found 1
./python/pfs/datamodel/guideStars.py:285:111: E501 line too long (135 > 110 characters)
./python/pfs/datamodel/guideStars.py:287:1: W391 blank line at end of file
The following tests failed:
/home/alefur/devel/datamodel/tests/.tests/linter-flake8.log.failed
1 tests failed
scons: *** [checkTestStatus] Error 1
scons: building terminated because of errors.


@alefur
Copy link
Contributor

alefur commented Mar 19, 2025

There is an issue when you read the config and write the config again.
This causes the fits writer to crash because it can't handle None. see error below.
I don't think None if actually the right default value if no flag were defined in the fits.
I think just use 0, meaning no flags are set.

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[11], line 1
----> 1 config.write('.')

File [~/devel/drp/datamodel/python/pfs/datamodel/pfsConfig.py:936](http://localhost:8888/home/alefur/devel/drp/datamodel/python/pfs/datamodel/pfsConfig.py#line=935), in PfsDesign.write(self, dirName, fileName, allowSubset)
    934 if fileName is None:
    935     fileName = self.filename
--> 936 self._writeImpl(os.path.join(dirName, fileName), allowSubset=allowSubset)

File [~/devel/drp/datamodel/python/pfs/datamodel/pfsConfig.py:911](http://localhost:8888/home/alefur/devel/drp/datamodel/python/pfs/datamodel/pfsConfig.py#line=910), in PfsDesign._writeImpl(self, filename, allowSubset)
    899 fits.append(pyfits.BinTableHDU.from_columns([
    900     pyfits.Column(name='fiberId', format='J', array=fiberId),
    901     pyfits.Column(name='fiberFlux', format='E', array=fiberFlux, unit='nJy'),
   (...)
    907     pyfits.Column(name='filterName', format='A%d' % maxLength, array=filterNames),
    908 ], hdr, name='PHOTOMETRY'))
    910 assert (self.guideStars is not None)
--> 911 self.guideStars.toFits(fits)
    913 # clobber=True in writeto prints a message, so use open instead
    914 with open(filename, "wb") as fd:

File [~/devel/drp/datamodel/python/pfs/datamodel/guideStars.py:168](http://localhost:8888/home/alefur/devel/drp/datamodel/python/pfs/datamodel/guideStars.py#line=167), in GuideStars.toFits(self, fits)
    150 header['TEL_ELEV'] = (self.telElev, "telescope elevation [degrees]")
    151 header['GS_CATID'] = (self.guideStarCatId,
    152                       "identifier of catalogue containing the guide stars")
    154 hdu = BinTableHDU.from_columns([
    155     Column("objId", "K", array=self.objId),
    156     Column("epoch", format='A7', array=self.epoch),
    157     Column("ra", "D", array=self.ra, unit='degrees'),
    158     Column("dec", "D", array=self.dec, unit='degrees'),
    159     Column("pmRa", "E", array=self.pmRa, unit='mas[/year](http://localhost:8888/year)'),
    160     Column("pmDec", "E", array=self.pmDec, unit='mas[/year](http://localhost:8888/year)'),
    161     Column("parallax", "E", array=self.parallax, unit='mas'),
    162     Column("magnitude", "E", array=self.magnitude),
    163     Column("passband", format='A%d' % maxLength, array=self.passband),
    164     Column("color", "E", array=self.color),
    165     Column("agId", "J", array=self.agId),
    166     Column("agX", "E", array=self.agX, unit='pixels'),
    167     Column("agY", "E", array=self.agY, unit='pixels'),
--> 168     Column("flag", "J", array=self.flag),
    169 ], header=header, name=self._hduName)
    170 fits.append(hdu)

File [/software/condaRoot/envs/rubin8-ics/lib/python3.11/site-packages/astropy/io/fits/column.py:717](http://localhost:8888/software/condaRoot/envs/rubin8-ics/lib/python3.11/site-packages/astropy/io/fits/column.py#line=716), in Column.__init__(self, name, format, unit, null, bscale, bzero, disp, start, dim, array, ascii, coord_type, coord_unit, coord_ref_point, coord_ref_value, coord_inc, time_ref_pos)
    712             else:
    713                 raise ValueError(
    714                     f"Data is inconsistent with the format `{format}`."
    715                 )
--> 717 array = self._convert_to_valid_data_type(array)
    719 # We have required (through documentation) that arrays passed in to
    720 # this constructor are already in their physical values, so we make
    721 # note of that here
    722 if isinstance(array, np.ndarray):

File [/software/condaRoot/envs/rubin8-ics/lib/python3.11/site-packages/astropy/io/fits/column.py:1432](http://localhost:8888/software/condaRoot/envs/rubin8-ics/lib/python3.11/site-packages/astropy/io/fits/column.py#line=1431), in Column._convert_to_valid_data_type(self, array)
   1427 # The .base here means we're dropping the shape information,
   1428 # which is only used to format recarray fields, and is not
   1429 # useful for converting input arrays to the correct data type
   1430 dtype = np.dtype(numpy_format).base
-> 1432 return _convert_array(array, dtype)

File [/software/condaRoot/envs/rubin8-ics/lib/python3.11/site-packages/astropy/io/fits/util.py:683](http://localhost:8888/software/condaRoot/envs/rubin8-ics/lib/python3.11/site-packages/astropy/io/fits/util.py#line=682), in _convert_array(array, dtype)
    681     return array.view(dtype)
    682 else:
--> 683     return array.astype(dtype)

TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

@alefur
Copy link
Contributor

alefur commented Mar 19, 2025

here is a proposed fix, It does insert correctly in the database.

@kiyoyabe
Copy link
Contributor Author

Thank you for the review!
I didn't see any problem when I ran the unit test. Now I got the following after running scons -Q -j 6 opt=3 .

=================================================================================================================================================================================================== slowest 5 durations ====================================================================================================================================================================================================

(5 durations < 0.005s hidden.  Use -vv to show these durations.)
================================================================================================================================================================================================= short test summary info ==================================================================================================================================================================================================
FAILED tests/test_GuideStars.py::GuideStarsTestCase::testFitsLargeObjId - TypeError: GuideStars.__init__() missing 1 required positional argument: 'flag'
FAILED tests/test_GuideStars.py::GuideStarsTestCase::testGoodLengths - TypeError: GuideStars.__init__() missing 1 required positional argument: 'flag'
FAILED tests/test_GuideStars.py::GuideStarsTestCase::testBasic - TypeError: GuideStars.__init__() missing 1 required positional argument: 'flag'
FAILED tests/test_GuideStars.py::GuideStarsTestCase::testFits - TypeError: GuideStars.__init__() missing 1 required positional argument: 'flag'
FAILED tests/test_GuideStars.py::GuideStarsTestCase::testBadLengths - TypeError: GuideStars.__init__() missing 1 required positional argument: 'flag'
============================================================================================================================================================================================== 5 failed, 6 warnings in 11.11s ==============================================================================================================================================================================================
Global pytest run: failed with 1

That's why I set default flag=None but should I rather change test_GuideStars.py? Or did I do something wrong?

@alefur alefur force-pushed the tickets/DAMD-173 branch 2 times, most recently from de2619e to c7e0bda Compare March 19, 2025 21:56
@alefur
Copy link
Contributor

alefur commented Mar 19, 2025

oups ! yes, you were right.
fixed the tests and flake8.

@alefur
Copy link
Contributor

alefur commented Mar 19, 2025

Does the obsproc scripts now pass in the flag to GuideStar constructor ?
If not maybe let's be nice and set a default value to None, as you did before.

 def __init__(self, objId, epoch, ra, dec,
                 pmRa, pmDec,
                 parallax,
                 magnitude, passband,
                 color,
                 agId, agX, agY,
                 telElev, guideStarCatId, flag=None):

        flag = np.zeros_like(objId, dtype='int32') if flag is None else flag

@kiyoyabe
Copy link
Contributor Author

Thank you! I think you proposal is good.

@kiyoyabe kiyoyabe merged commit 5941af5 into master Mar 20, 2025
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