phoebe-project / phoebe2

PHOEBE - Eclipsing Binary Star Modeling Software
http://phoebe-project.org
GNU General Public License v3.0
76 stars 28 forks source link

Interpolation/extrapolation switch to a C module #826

Closed aprsa closed 3 months ago

aprsa commented 4 months ago

This is a complete rewrite of the ndpolator in C. There are a few more things I'd like to optimize but the code is fully functional and it passes all tests. As the first step, I'd kindly ask for the review of dependencies/ndpolator code. It is C but it is heavily documented. You can generate fully documentation using doxygen. @Raindogjones, the treatment of down-dimensioning has been reworked, so please see if this fixes the issue you were seeing before. I am leaving this in draft stage for now as we will likely need a few iterations to get right. This is a HUGE diff so brace yourselves...

aprsa commented 4 months ago

Ignore test failures, they're all benign: we need to rebase to 2.4.13 once out (which will fix 3.7 failures).

kecnry commented 4 months ago

running for the default binary, I'm not seeing:

File ~/repos/phoebe2/phoebe/frontend/bundle.py:11929, in Bundle.run_compute(self, compute, model, solver, detach, dataset, times, return_changes, **kwargs)
  11927 if 'lc' in ds_kinds_enabled or 'rv' in ds_kinds_enabled or 'lp' in ds_kinds_enabled:
  11928     logger.info("run_compute: computing necessary ld_coeffs, pblums, l3s")
> 11929     self.compute_ld_coeffs(compute=compute, skip_checks=True, set_value=True, **{k:v for k,v in kwargs.items() if k in computeparams.qualifiers})
  11930     # NOTE that if pblum_method != 'phoebe', then system will be None
  11931     # otherwise the system will be create which we can pass on to the backend
  11932     # the phoebe backend can then skip initializing the system at least on the master proc
  11933     # (workers will need to recreate the mesh)
  11934     system, pblums_abs, pblums_scale, pblums_rel, pbfluxes = self.compute_pblums(compute=compute, ret_structured_dicts=True, skip_checks=True, **{k:v for k,v in kwargs.items() if k in computeparams.qualifiers})

File ~/repos/phoebe2/phoebe/frontend/bundle.py:10328, in Bundle.compute_ld_coeffs(self, compute, set_value, **kwargs)
  10324 query_pts = np.array([[teff, logg, abun],])
  10326 # interpolate_ldcoeffs() always returns an array, so we need
  10327 # the first element of the array.
> 10328 ld_coeffs = pb.interpolate_ldcoeffs(
  10329     query_pts=query_pts,
  10330     ldatm=ldcs,
  10331     ld_func=ld_func,
  10332     intens_weighting=intens_weighting,
  10333     ld_extrapolation_method=ld_extrapolation_method
  10334 )[0]
  10336 # NOTE: these may return nans... if so, run_checks will handle the error
  10338 logger.info("interpolated {} ld_coeffs{}={}".format(ld_func, bol_suffix, ld_coeffs))

File ~/repos/phoebe2/phoebe/atmospheres/passbands.py:1385, in Passband.interpolate_ldcoeffs(self, query_pts, ldatm, ld_func, intens_weighting, ld_extrapolation_method)
   1382 if f'{ldatm}:ld' not in self.content:
   1383     raise ValueError(f'Limb darkening coefficients for ldatm={ldatm} are not available; please compute them first.')
-> 1385 ld_coeffs = self.ndp[ldatm].ndpolate(f'ld@{intens_weighting}', query_pts, extrapolation_method=ld_extrapolation_method)
   1386 return ld_coeffs[s[ld_func]]

File ~/repos/phoebe2/phoebe/dependencies/ndpolator/ndpolator.py:94, in Cndpolator.ndpolate(self, table, query_pts, extrapolation_method)
     92 capsule = self.table[table][2]
     93 if capsule:
---> 94     interps = cndpolator.ndpolate(capsule=capsule, query_pts=query_pts, nbasic=len(self.axes), extrapolation_method=extrapolation_method)
     95 else:
     96     attached_axes = self.table[table][0]

TypeError: function missing required argument 'axes' (pos 2)
aprsa commented 4 months ago

Can someone confirm this? I cannot reproduce on my end, and all checks are passing on CI as well. @kecnry, could it be that cndpolator wasn't rebuilt properly?

kecnry commented 4 months ago

Manually rebuilding fixed it, whew!

Raindogjones commented 4 months ago

I just pushed a couple of small updates so that the required changes for the new TMAP grids are already there (more to make my life easier with the testing than anything else). Feel free to reverse the commits if you think they should be kept separate, but they don't change much other than splitting TMAP into four grids.

aprsa commented 3 months ago

@kecnry, @Raindogjones, as you might have noticed, and in line with what was discussed at the telecon yesterday, I separated ndpolator from phoebe. The latest commit passes all the tests, so I think that the only remaining issue is the fits keyword length. Once that is done, I'll convert this draft PR to an actual PR and we can merge it in.

Raindogjones commented 3 months ago

I'm struggling to think of a good/clear abbreviation, but maybe this could work: TA = TMAP DA TO = TMAP DO TS = TMAP sdO TM = TMAP DAO @aprsa?

aprsa commented 3 months ago

TA = TMAP DA TO = TMAP DO TS = TMAP sdO TM = TMAP DAO

@Raindogjones, works for me!

Raindogjones commented 3 months ago

Requested TMAP changes made (plus references). @aprsa will need to rebuild the default bundles and passbands but then I think that is it!

kecnry commented 3 months ago

Once the shipped passbands are updated the errors should go away, but I'm curious if our warnings to update are sufficient or if we're now going to have to require updating all passbands when updating phoebe. Is there anyway we can keep this backwards compatible?

aprsa commented 3 months ago

Once the shipped passbands are updated the errors should go away, but I'm curious if our warnings to update are sufficient or if we're now going to have to require updating all passbands when updating phoebe. Is there anyway we can keep this backwards compatible?

We won't need to require the update. This is only because the stock Johnson V passband didn't have blackbody extinction incorporated before (which in and of itself was an oversight). I'll edit the test that's failing and account for this omission, but otherwise there will be no adverse effects. We remain backwards-compatible.