Today we had the discussion associated with RFC-123. I took notes during said discussion. I take responsibility for any errors in the following.
I’ll start with Simon’s executive summary, for the TL:DR folks:
Simon’s Summary:
We are going to try to preserve the API as it exists, so we will maintain internal dependencies and replace them piecemeal. We cannot make headway without: persistable DistortedWcs, persistable BoundedField (to be used to save the photometric scaling model, e.g. simphotom). Everything else requires the WCS rethink, but we should be able to merge in simastrom before then.
Jim: it would be good to also do the same for photometry.
Pierre: simastrom has a photometry fit internally, but no way to write it out.
Jim: there is an object for this in HSC that we do not want, but there’s a new object for aperture corrections (BoundedField) that we could use for this.
Simastrom RFD
Goal of discussion
Getting John P. up to speed on the code.
Changes necessary to merge with the stack
Changes necessary to merge with HSC code
Integrating into the stack
Jim’s comments
Jim Bosch started by going into more detail on his comments on the ticket:
add simastrom primitives to afw DM-4044
improve usage of boost pointers DM-4043
once we switch to shared_ptr, we can drop boost
intrusive_ptr is safer,w hich is why we want to use it
Gtransfo->XYTransform integration DM-4045
Needs someone to think through the design
Good to think about as part of WCS rewrite: this would be a core peice of a new WCS system.
Simon: Could this be a wrapper around a library of transforms?
Jim: there should be a relationship between the class heirarchies. The same mathematical transform parameterize in different ways.
We don’t have a way to fit XY transforms in the stack currently.
The interface, and the fitter, are separate problems.
Pierre: In order for the fitter to be stable, you have to be able to compose them.
Do we want to be able to mathematically collapse the polynomials, or just lazily use them separately.
Simastrom reports the composition of polynomials, which is not exactly what was fit.
Jim: the output of simastrom should be the final composition, or allow the output of all the polynomials.
Pierre: simastrom does not approximate during composition: composed polynomials are exact.
Currently don’t have a way to save the more complex outputs of simastrom, as there is no way to represent them.
Robert: we need to determine how to represent the full complex models, and we need to do it now. Can’t keep punting on this.
The fundamental question: we need to figure out how we are going to represent the WCS.
WCS representation needs to take into account simastrom requirements.
minimal complex focal plane model persistence (NEEDS TICKET!)
We need an n-way mapper like Associations from simastrom (NEEDS TICKET!)
simultaneous photometry
exists in meas_mosaic
Don’t have any way to represent them yet.
no current standard in astronomy, so we’re not fighting against an existing standard.
It’s more than a zero-point for every chip: there are other polynomial corrections that are included.
Yusra ported something from HSC, but we need to make sure they are all parameterized the same way.
Robert: worried that we’re carrying over a pragmatic solution, which is not the solution we actually want.
Pierre’s comments
A problem is getting the initial map for the entire mosaic.
Currently get failures possibly because the WCS from the initial match are wrong.
The technical problem is not the match itself, but the way we persist the match.
Robert: we scatter (process each CCD) and gather (merge across the mosaic)
Jim: the functionality for this exists in the HSC fork.
Simon’s summary
Lots of work to do.
Need to make things persisteable
Before we can persist, we need to define how the WCS will be represented.
What headway can we make before we solve the above?
Jim: we can probably define something with our WCS interface that we can persist into a FITS binary table (which we can already do). We can then read that back in exactly.
We’re talking about FITS here because that’s the path of least resistence for persistence (but everything happens in afw, so the IO should be independent).
We may want a way to output simple FITS-based WCS headers specifically for ds9 or other viewers. We don’t need it for anything else.
We currently can’t persist the distorted WCS.
It’s not clear that simastrom’s wcs can be represented by a distorted WCS on top of a TAN WCS.
Currently: simastrom uses TAN, but it is not necessary. So, we’re ok here.
We don’t support TPV or TNX, but we don’t need to stick to SIP.
Remember: we’re talking about our use of external tools here.
The fastest solution is to just write something that works now, even if it is SIP.
Note that this may be useful elsewhere because it may be much more efficient when doing warping, to have an approximate solution.
OSX support
Cannot build the dependencies on OSX, because of librt requirement in SuiteSparse/micro_cholmod.
Cholmod is used because it supports outlier removal.
Eigen cholmod is supposed to be an interface to SuiteSparse.
micro_cholmod is supposed to be independent of LAPACK
Eigen delegates to a user-installed suitesparse as an option.
We wouldn’t gain anything by adding this functionality to Eigen, since we’d still be relying on SuiteSparse.
We can find someone in France to look into it.
We don’t use SuiteSparse in the stack currently.
micro_cholmod is supposed to be the minimal library.
Domnique: the performance difference is huge.
Installation process
Notes on getting it to work are scattered around on DESC pages for example. These should be consolidated.
Note that you need to setup both the package, and it’s sub-directory astrometry_net_data/ to get all the necessary pieces.
package name
“simultaneous astrometry” isn’t a bad name choice. The only problem with the name is that “sim” is an overloaded term. We should consider that when merging it back into the stack.
If eigen supported ldupdate from cholmod could we use it? It wouldn’t help with the build question (eigen would need to be linked with at least parts of sparsesuite), but it would mean that we only had one matrix front end. I’ve pinged the eigen list.
I think @jbosch thinks that we may need to switch to e.g. Mike Jarvis’s TMV package if we go the openMP route, but I think that’s a different question.
I am wondering if including the subpart of Cholmod needed for the astrometric fit into the simastrom package
is a good idea. At the moment the needed stand-alone subpart of Cholmod is proposed in a separate
package (micro_cholmod) extracted from SuiteSparse, and not depending of lapack and blas.
If it seems unlikely that other parts of DM will rely on the Cholesky factorization update feature,
it would be simpler both for the maintenance and the installation to just put the needed
cholmod routines (with proper attribution obviously) into simastrom, thus delivering
simastrom a single package without any new dependencies.
I’m inclined to leave this as it is for now. There are some other areas of the stack where I could imagine us doing sparse matrix algebra in the future. More importantly we may want to revisit our decision to avoid LAPACK/BLAS in favor of Eigen when we consider switching to lightweight threads for parallelization, and that could make just building all of suitesparse as a third-party package desirable.
Basically, I think the future is sufficiently uncertain in this area that we should just do what’s easiest for now, and I assume that’s the status quo.
A question for the experts : is it possible to enforce -O3 or higher for micro_chomod and simastrom? I managed to change the opt value inside lib/SConscript, but then I did not find a way to change it back for the rest of the build…
Elaborating a bit :
What I have done as a quick test is to add inside lib/SConscript:
if ‘-O0’ in env[‘CCFLAGS’]:
env[‘CCFLAGS’].remove(’-O0’)
env[‘CCFLAGS’].append(’-O3’)
Clearly this is bad, as the actual meaning of opt in the context of gcc compilation is exposed. What I would like to do is something along the following line:
if not ‘opt’ in env : #do not discard explicit command line option provided by user
env.set_opt(3) #change default to be 3 instead of 0
The default opt=0 is not a good idea for this package, so we would like to have opt=0 only if the user explicitly sets it so at the command line. So yes, opt=3 works but precisely, it has to be added to counteract opt=0 default. Is it possible to set differing defaults for specific packages? Can a package define its own defaults? Does not sound ludicrous to me :)…
I would be in favor of changing the default to opt=3 more globally. I believe it is already the default for all “official” builds (like anything installed by eups distrib install), and I think perhaps it should be the default in all sconsUtils builds.
All C++ code needs to be built fully optimised except when you are debugging. I don’t think we want to treat this package differently.
As @jbosch says, we routinely compile fully optimised. If you want to do “private” builds the same way, you can set the environment variable SCONSFLAGS to include “opt=3”.
I wouldn’t object to setting the LSST system-wide default to opt=3, as you should still be able to override it on the command line. afw and meas_algorithms also support “noOpt=filename” to build just one file (or a set of files) unoptimised. Feel free to file an RFC
When you run scons manually, it will always pay attention to SCONSFLAGS, not EUPSPKG_SCONSFLAGS. My (limited) understanding of future versions of eupspkg is that they will not propagate SCONSFLAGS directly, but will propagate EUPS_SCONSFLAGS into SCONSFLAGS when running scons.
I’m not sure where the opt=3 default in eupspkg (and other) builds is now.