jveitchmichaelis / rascal

RAnsac Assisted Spectral CALibration
BSD 3-Clause "New" or "Revised" License
12 stars 2 forks source link

Plotting error in plot_fit in plotting.py #51

Closed buckleygeer closed 2 years ago

buckleygeer commented 2 years ago

Hi In the plot.fit function there is the following code (lines 604-613)

        ax1.vlines(
            calibrator.polyval(calibrator.peaks, fit_coeff),
            np.array(spectrum)[
                calibrator.pix_to_rawpix(calibrator.peaks).astype("int")
            ],
            vline_max,
            linestyles="dashed",
            colors="C1",
            label="Detected Peaks",
        )

I assume that the second argument is trying to treat the peaks values as integers to pass to pix_to_rawpix which is a call to interpolate.interp1d. However it doesn't actually seem to be doing this because in one of my spectra I have a peak value of 3131.02031368 and when the interpolate code compares that value against the value of 3131 I get a bounds check error from the interp1d code. I put in print statements so I could see what was happening because interp1d doesn't actually tell you what the out-of-bounds value is! I think the code needs to be (it doesn't crash when I make that change)

        ax1.vlines(
            calibrator.polyval(calibrator.peaks, fit_coeff),
            np.array(spectrum)[
                calibrator.pix_to_rawpix(calibrator.peaks.astype("int").astype("int")
            ],
            vline_max,
            linestyles="dashed",
            colors="C1",
            label="Detected Peaks",
        )

Thanks Liz

jveitchmichaelis commented 2 years ago

Thanks, I'll take a look!

I think we'd only need to cast to int once though.

buckleygeer commented 2 years ago

Thanks. I confess that it took me a while to figure out what the code was doing and I am still not totally sure I understand it. So I don’t totally understand why that change made it work.

Liz

buckleygeer commented 2 years ago

So I don't think I fixed the problem as I just had it fail in the same way on a different spectrum. I think that because the fits are not deterministic I get different peaks each time I run it and I must have got a different set of peaks which made me think I had fixed the problem. So yes you are right, casting twice makes no difference to the problem.

buckleygeer commented 2 years ago

So after thinking about this further I realize that an easy solution is to just eliminate the peaks that have values that exceed the upper interpolation limit.

cylammarco commented 2 years ago

Hi @buckleygeer, I think changing the interpolator argument to have fill_value='extrapolate' in line 1014-1016 of calibrator.py should solve the problem.

It is now reflected in the most recent dev commit. If that fixes the problem, we can quickly roll out a patch.

buckleygeer commented 2 years ago

Thanks, I’ll try that.

Liz

From: Marco @.> Reply-To: jveitchmichaelis/rascal @.> Date: Monday, June 20, 2022 at 4:01 PM To: jveitchmichaelis/rascal @.> Cc: Elizabeth Buckley-Geer @.>, Mention @.***> Subject: Re: [jveitchmichaelis/rascal] Plotting error in plot_fit in plotting.py (Issue #51)

Hi @buckleygeerhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_buckleygeer&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=uQLDQQYHpNrb5Y1LVgizsovggLjKhpVIIme_fVsDbjA&m=zMHjotkNTrhL8AdHBBXZjd6y0_xz8yOG2lBidLJpX33EVrfcZPxS6DxzrfqyqI5A&s=ZgVtcRwFKuaVq9vSZg2DwB4hkBGJBJa-eDTdWeahxH0&e=, I think changing the interpolator argument to have fill_value='extrapolate' in line 1014-1016 of calibrator.py should solve the problem.

— Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jveitchmichaelis_rascal_issues_51-23issuecomment-2D1160843609&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=uQLDQQYHpNrb5Y1LVgizsovggLjKhpVIIme_fVsDbjA&m=zMHjotkNTrhL8AdHBBXZjd6y0_xz8yOG2lBidLJpX33EVrfcZPxS6DxzrfqyqI5A&s=-R25IF7MZQ2cx1jf6OOpLqATT7w3s1BSms_YMCzL7wg&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ADXFMBAR3HKFRCV24ISERLDVQDL3BANCNFSM5WDBOLKQ&d=DwMCaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=uQLDQQYHpNrb5Y1LVgizsovggLjKhpVIIme_fVsDbjA&m=zMHjotkNTrhL8AdHBBXZjd6y0_xz8yOG2lBidLJpX33EVrfcZPxS6DxzrfqyqI5A&s=EMQQjCt4C3YFjDX-wZL1_SIqaCOw1GZSByHSWQDX81s&e=. You are receiving this because you were mentioned.Message ID: @.***>

cylammarco commented 2 years ago

Assume the patch works.