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

Add ape_e spectral factor model #2126

Closed
wants to merge 3 commits into from
Closed

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Jul 11, 2024

This function calculates the spectral mismatch factor using the average photon energy and the depth of a water absorption band as inputs. Context: #2065

update __init__.py, mismatch.py, test_spectrum
@RDaxini RDaxini marked this pull request as draft July 11, 2024 15:30
Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

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

Nice job with this implementation!

I miss a test of numpy input/output datatypes. Also, sphinx warns something near line 64 of the documentation, but I can't grasp what it could be.

Comment on lines +861 to +865
The default is None.

coefficients : array-like, optional
User-defined coefficients, if not using one of the default coefficient
sets via the ``module_type`` parameter. The default is None.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove "The default is None."?

#1574 , #1828, #2084

Average photon energy (APE, :math:`\varphi`) [eV]

band_depth : numeric
Depth of a spectral band, :math:`\varepsilon` [Wm:math:`^{-2}`]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Depth of a spectral band, :math:`\varepsilon` [Wm:math:`^{-2}`]
Depth of a spectral band, :math:`\varepsilon` [W/m²]

out = spectrum.spectral_factor_daxini(apes, bands,
module_type=module_type)
assert isinstance(out, pd.Series)
assert np.allclose(expected, out, atol=1e-5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert np.allclose(expected, out, atol=1e-5)
assert_allclose(out, expected, atol=1e-5)

@kandersolar kandersolar added the GSoC Contributions related to Google Summer of Code. label Jul 22, 2024
@RDaxini
Copy link
Contributor Author

RDaxini commented Jul 25, 2024

@echedey-ls Thanks for the review.
I am waiting for a conclusion to the discussion in #2065 before working on (or closing...) this PR.

@RDaxini
Copy link
Contributor Author

RDaxini commented Jul 26, 2024

Closed for now, see discussion in #2065 regarding availability of coefficients for more details.

@RDaxini RDaxini closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement GSoC Contributions related to Google Summer of Code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants