Updates to accommodate ndpolator 1.2.1. #912

aprsa · 2024-06-26T11:28:45Z

The code has also been further simplified and debugged, and several deprecations have been handled. For the time being, numpy <2.0 is imposed until all tests are run adequately.

Raindogjones · 2024-07-05T09:51:56Z

The updates to handle properly blackbody extinction seem to work well (I've just submitted a PR to Andrej's branch which should fix model atmosphere extinction too).

I am however finding differences between the new passband tables and old ones. Would be good to talk about why that is (the change in function used to integrate ptf and sed?) and decide whether this changes are sufficiently appreciable to warrant further investigation.

kecnry · 2024-07-05T13:03:24Z

Are any of the other changes (simplifications, debugging, numpy max pin) relevant to other branches? If so, can you please move them into their own PRs off the correct branch?

Raindogjones · 2024-07-05T13:14:19Z

Not on my part - they are all related to the rewrite of passbands.py for v2.5

aprsa · 2024-08-27T01:38:03Z

@kecnry, the diff is only really pertinent to the blending branch. We can do the numpy version cap independently in the 2.4.x, because here it's also tied to the ndpolator version. That can be a separate PR.

kecnry

We can do the numpy version cap independently in the 2.4.x, because here it's also tied to the ndpolator version. That can be a separate PR.

Ok, let's not forget to do that then and we'll just deal with any conflicts (they should be easy to sort out if its limited to just that)

kecnry · 2024-08-27T12:26:28Z

phoebe/atmospheres/passbands.py

+            # ebv_column = np.tile(ebvs, len(rvs))
+            # rvebv_column = ebv_column*np.repeat(rvs, len(ebvs))
+            # ext_pars = np.vstack((rvebv_column, ebv_column))
+            # ext_func = libphoebe.gordon_extinction(wls)  # (47, 2)
+            # ext = ext_func @ ext_pars  # (47, 480)
+            # flux_fracs = 10**(-0.4*ext)  # (47, 480)
+            # integrand_energy = (pbpfs_energy[:, None, :]*flux_fracs[:, :, None]).T  # ~25ms  (97, 480, 47)
+            # integrand_photon = (pbpfs_photon[:, None, :]*flux_fracs[:, :, None]).T  # ~25ms  (97, 480, 47)
+
+            # extincted_intensities_energy = np.trapz(integrand_energy, self.wl, axis=2)  # (97, 480)
+            # extincted_intensities_photon = np.trapz(integrand_photon, self.wl, axis=2)  # (97, 480)
+
+            # non_extincted_intensities_energy = np.trapz(pbpfs_energy, self.wl, axis=0)[:,None]  # (97, 1)
+            # non_extincted_intensities_photon = np.trapz(pbpfs_photon, self.wl, axis=0)[:,None]  # (97, 1)
+
+            # egrid = (extincted_intensities_energy/non_extincted_intensities_energy).reshape(len(teffs), len(ebvs), len(rvs), 1)  # (97, 16, 30, 1)
+            # pgrid = (extincted_intensities_photon/non_extincted_intensities_photon).reshape(len(teffs), len(ebvs), len(rvs), 1)  # (97, 16, 30, 1)
+
+            # self.ndp['blackbody'] = ndpolator.Ndpolator(basic_axes=(axes[0],))
+            # self.ndp['blackbody'].register('ext@photon', associated_axes=(axes[1], axes[2]), grid=pgrid)
+            # self.ndp['blackbody'].register('ext@energy', associated_axes=(axes[1], axes[2]), grid=egrid)
+


can this just be removed? It will always exist in git history anyways

Suggested change

# ebv_column = np.tile(ebvs, len(rvs))

# rvebv_column = ebv_column*np.repeat(rvs, len(ebvs))

# ext_pars = np.vstack((rvebv_column, ebv_column))

# ext_func = libphoebe.gordon_extinction(wls) # (47, 2)

# ext = ext_func @ ext_pars # (47, 480)

# flux_fracs = 10**(-0.4*ext) # (47, 480)

# integrand_energy = (pbpfs_energy[:, None, :]*flux_fracs[:, :, None]).T # ~25ms (97, 480, 47)

# integrand_photon = (pbpfs_photon[:, None, :]*flux_fracs[:, :, None]).T # ~25ms (97, 480, 47)

# extincted_intensities_energy = np.trapz(integrand_energy, self.wl, axis=2) # (97, 480)

# extincted_intensities_photon = np.trapz(integrand_photon, self.wl, axis=2) # (97, 480)

# non_extincted_intensities_energy = np.trapz(pbpfs_energy, self.wl, axis=0)[:,None] # (97, 1)

# non_extincted_intensities_photon = np.trapz(pbpfs_photon, self.wl, axis=0)[:,None] # (97, 1)

# egrid = (extincted_intensities_energy/non_extincted_intensities_energy).reshape(len(teffs), len(ebvs), len(rvs), 1) # (97, 16, 30, 1)

# pgrid = (extincted_intensities_photon/non_extincted_intensities_photon).reshape(len(teffs), len(ebvs), len(rvs), 1) # (97, 16, 30, 1)

# self.ndp['blackbody'] = ndpolator.Ndpolator(basic_axes=(axes[0],))

# self.ndp['blackbody'].register('ext@photon', associated_axes=(axes[1], axes[2]), grid=pgrid)

# self.ndp['blackbody'].register('ext@energy', associated_axes=(axes[1], axes[2]), grid=egrid)

Let's leave these in for now because Dave's workaround increases the time cost by an order of magnitude. It'd be better to fix this code than to rely on the much slower implementation. I'll remove the comments before the final PR.

phoebe/atmospheres/passbands.py

kecnry · 2024-08-27T12:29:59Z

phoebe/atmospheres/passbands.py

+
+        ints = {
+            'inorms': intensities
+            # TODO: add other dict keys!


does this need to be done now? Is there a ticket filed otherwise?

that's coming up next: it will contain the amount of blending.

kecnry · 2024-08-27T12:31:18Z

phoebe/atmospheres/passbands.py

+                # anything else we need to special-handle for ld_func == 'interp'?
+                pass


is this case tested anywhere? Are any variables not assigned that will cause code to raise an error?

this task was meant to be offloaded to a test suite (in the making).

what would get to this else? An unsupported value for atm?

any combination of atm and ldatm that isn't explicitly handled, including invalid atm/ldatm values. Instead of pass we could raise an error but I figured I'd write a test case that goes through a cartesian product of all atmospheres for atm and ldatm, including non-existing ones, and see if anything reaches this block.

ok, can you please make sure there is a ticket for those test cases so they aren't forgotten?

kecnry · 2024-08-27T12:32:09Z

phoebe/atmospheres/passbands.py

+            mus = query_pts[:,-1]
+            reduced_query_pts = query_pts[:,:-1]
+
+            # print(f'{query_pts=}, {mus=}, {atm=}, {ldatm=}, {ldint=}, {ld_func=}, {ld_coeffs=}, {intens_weighting=}, {atm_extrapolation_method=}, {ld_extrapolation_method=}, {blending_method=}, {return_nanmask=}')


is this useful to keep for future debugging? Would it be useful in the debug logger?

probably not in the debug logger, but certainly for now as we test the implementation. It will be removed in the final PR.

phoebe/backend/universe.py

kecnry

I think this is good to merge - just please make any follow-up tickets so we can track what we still want to do before this effort is finalized!

kecnry · 2024-10-08T12:38:27Z

phoebe/atmospheres/passbands.py

+                # anything else we need to special-handle for ld_func == 'interp'?
+                pass


ok, can you please make sure there is a ticket for those test cases so they aren't forgotten?

The code has also been further simplified and debugged, and several deprecations have been handled. For the time being, numpy <2.0 is imposed until all tests are run adequately.

This commit: * unoptimizes extinction computation but greatly improves code reliability; the price to pay is 0.2s -> 1.8s, so almost an order of magnitude, but a small order of magnitude. ;) We can consider having both versions supported so that comparing them is readily available; * fixes a bug in feature-blending where E(B-V) and Rv for blackbody atmospheres have been reversed; * makes extinction interpolation smart so that it can deal with extra columns between teffs and ebvs, rvs (which is usual if we have loggs and abuns); * adds extinction to the ignored effects for pblum computation; * removes needless ascontiguousarray() casts; * simplifies the code for adding columns to query_pts in _populate_lc(); * removes commented out print() debugging calls; and * fixes the blackbody extinction test that also had ebvs and rvs reversed. The commit does not touch the model atmosphere part of the code; passbands need to be recomputed for blackbody extinction tables. All tests are passing locally.

kecnry · 2024-10-08T18:24:21Z

looks like something went wrong during the rebase and tests are now failing 😬 Hopefully easy to fix 🤞

aprsa requested a review from kecnry June 26, 2024 11:28

kecnry reviewed Aug 27, 2024

View reviewed changes

aprsa self-assigned this Oct 4, 2024

kecnry approved these changes Oct 8, 2024

View reviewed changes

aprsa and others added 7 commits October 8, 2024 13:58

Updates to accommodate ndpolator 1.2.1.

1e54f4b

The code has also been further simplified and debugged, and several deprecations have been handled. For the time being, numpy <2.0 is imposed until all tests are run adequately.

First stab (#6)

a14b0f1

Cleaned up version of the extinction fix.

a60a394

Fixes a typo that caused specific intensities to return None

becb72c

Addresses @kecnry's comments.

3f31f63

Implements fractional blending information for Inorm

fa84de4

aprsa force-pushed the ndpolator-1.2.1_update branch from 0360b52 to fa84de4 Compare October 8, 2024 18:00

Fixes a small merge issue.

d510179

aprsa merged commit 5eddf48 into phoebe-project:feature-blending Oct 8, 2024
19 checks passed

aprsa deleted the ndpolator-1.2.1_update branch October 8, 2024 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to accommodate ndpolator 1.2.1. #912

Updates to accommodate ndpolator 1.2.1. #912

aprsa commented Jun 26, 2024

Raindogjones commented Jul 5, 2024

kecnry commented Jul 5, 2024

Raindogjones commented Jul 5, 2024

aprsa commented Aug 27, 2024

kecnry left a comment

kecnry Aug 27, 2024

aprsa Oct 4, 2024

kecnry Aug 27, 2024

aprsa Oct 4, 2024

kecnry Aug 27, 2024

aprsa Oct 4, 2024

kecnry Oct 4, 2024

aprsa Oct 4, 2024

kecnry Oct 8, 2024

kecnry Aug 27, 2024

aprsa Oct 4, 2024

kecnry left a comment

kecnry Oct 8, 2024

kecnry commented Oct 8, 2024

		# anything else we need to special-handle for ld_func == 'interp'?
		pass

Updates to accommodate ndpolator 1.2.1. #912

Updates to accommodate ndpolator 1.2.1. #912

Conversation

aprsa commented Jun 26, 2024

Raindogjones commented Jul 5, 2024

kecnry commented Jul 5, 2024

Raindogjones commented Jul 5, 2024

aprsa commented Aug 27, 2024

kecnry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry commented Oct 8, 2024