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

Addresses issue #416 AND recalculates bias step TES parameter values #424

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RemingtonGerras
Copy link
Contributor

Bias Step TES parameter values are calculated from the RP curve that satoru worked on!

…by calculating RP curve from satoru's work.
Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

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

Hi Remy, thanks for this!! Its very nice code, and works well for the testing I've done on existing observations. I have some inline comments with things that are coming up as I'm testing with existing data, and thinking about how it should integrate with the det_cal db script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs either a link to Satoru's confluence page describing the math, or better yet an RST page in the sodetlib docs dedicated to this analysis.

r1 = np.linspace(min(minrfrac, 0.0001), max(maxrfrac, 0.9999), 9999)
dr = r1[1] - r1[0]

def model_Pj(r, logp0=0., p1=7., p2=1., logp3=1.,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document here the mathematical description of the model function (or reference satoru's page), and also document the params?

np.seterr(all="ignore")


def translate_tes_params(iv_file, bs_file, save_iv=False, save_bs=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of passing file paths, can you accept pre-loaded iva and bsa dicts? This is more versatile and plugs in better with how we use it in the det_cal scripts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Returned values (iva2, bsa2) should start as deepcopies of the provided dicts.

np.seterr(all="ignore")


def translate_tes_params(iv_file, bs_file, save_iv=False, save_bs=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also you don't need to worry about saving files, so I'd remove the args and all logic pertaining to saving the data.

Comment on lines +137 to +139
R = iva2['R_V1'][i]
R_n = iva2['R_n_V1'][i]
p_tes = iva2['p_tes_V1'][i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

For these, and all other corrected values in both the iva and bsa objects, can you replace the existing keys (of the new deepcopied dicts) instead of adding new ones?

# 2. Recompute R, Si, Pj, and I0 based on RP_cuve shifted by delta_Popt
# 3. Save updated BSAnalysis object as a numpy file.
###############################################################################
def rerun_analysis(bs_file, iva2=None, save=False, R0_thresh=30e-3):
Copy link
Collaborator

Choose a reason for hiding this comment

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

iva2 should just be a positional argument

Comment on lines +36 to +37
iva2 = reanalyze_iv(iv_file, save_iv)
bsa2 = rerun_analysis(bs_file, iva2, save_bs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

To both the iva and bsa objects, can you add a new key version which should be set to 1? Then this can be checked to make sure corrections aren't re-applied to already corrected results.

@jlashner jlashner mentioned this pull request Aug 23, 2024
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.

2 participants