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

Create a BasicMathNode that can evaluate expressions given as a string #185

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

jeremykubica
Copy link
Contributor

Creates a new BasicMathNode that will evaluate basic math functions given as a string. Performs a bunch of checks to prevent arbitrary code execution. Also translates math functions into the correct backend (math, numpy, or jax).

This will allow us to write functions nodes (for basic math) as:

x0_node = BasicMathNode(
    "pow(10.0, -0.4 * (distmod - alpha * x1 + beta * c + m_abs - 10.635))",
    distmod = host.distmod,
    alpha=...
    ...
)

@jeremykubica jeremykubica requested a review from hombit November 4, 2024 15:34
Copy link

github-actions bot commented Nov 4, 2024

Before [a5b5410] After [788080a] Ratio Benchmark (Parameter)
4.51±0.05ms 4.81±0.2ms 1.07 benchmarks.TimeSuite.time_evaluate_salt3_passbands
8.35±0.05ms 8.69±0.6ms 1.04 benchmarks.TimeSuite.time_evaluate_salt3_model
698±30μs 716±700μs 1.03 benchmarks.TimeSuite.time_fnu_to_flam
4.43±0.08ms 4.51±0.3ms 1.02 benchmarks.TimeSuite.time_chained_evaluate
1.32±0.05ms 1.33±0.1ms 1.01 benchmarks.TimeSuite.time_apply_passbands
8.84±0.05ms 8.94±0.05ms 1.01 benchmarks.TimeSuite.time_load_passbands
1.35±0.01s 1.35±0.01s 1 benchmarks.TimeSuite.time_make_x1_from_hostmass
16.1±0.3μs 16.2±0.2μs 1 benchmarks.TimeSuite.time_sample_x1_from_hostmass
123±2μs 122±1μs 0.99 benchmarks.TimeSuite.time_sample_x0_from_distmod
28.5±0.4μs 27.9±0.3μs 0.98 benchmarks.TimeSuite.time_make_new_salt3_model

Click here to view all benchmarks.

from tdastro.base_models import FunctionNode


class BasicMathNode(FunctionNode):
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally irrelevant to this PR, but as a grad student I've definitely wished for this sort of conversion as a callable library, especially if C stdlib was one of the "basic math" languages that could be converted.

Copy link
Contributor

@hombit hombit left a comment

Choose a reason for hiding this comment

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

Some general usability issues, most of them are not related to this PR and can be discussed separately:

  1. BasicMathNode doesn't have a string representation
  2. GraphState doesn't have a string representation and iteration over it fails, e.g. list(BasicMathNode("a", a=1, node_label="node").sample_parameters())
  3. BasicMathNode("a", a=1).sample_parameters() cannot be used with [] syntax because node_label is None. However I still could use extract_parameters
  4. Math exceptions like 1/0 or log(-1) are hard to track
  5. It is not clear how I can a) jit-compile with jax and b) use a random number node.

@jeremykubica
Copy link
Contributor Author

Thanks @hombit, I filed some issues for some of those:

  1. I need to think about how to provide a string representation, because I currently use __str__ for some of the node labeling.
  2. Added issue Make GraphState iterable #186
  3. Added issue Create a connivence accessor for a single node GraphState #187
  4. Agreed. I don't have a great solution here, but I can try adding better debugging (Add better exception handling for math errors #188)
    5a) I don't think jit compiling is possible with this node, so we would need to be careful.
    5b) You can use a random variable as input the same way as within any other node.

@jeremykubica jeremykubica merged commit 1cd7636 into main Nov 7, 2024
5 checks passed
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.

4 participants