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

fix: bind parameters in models with frozen parameters #118

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

phajy
Copy link
Contributor

@phajy phajy commented Jun 26, 2024

When binding parameters we should only be considering free parameters in the model, not all parameters. Updated _get_index_of_symbol in binding.jl to skip frozen parameters.

Added a couple of test cases to check this works. Could probably add more tests.

@phajy
Copy link
Contributor Author

phajy commented Jun 26, 2024

Even though the change is only really one functional line it took me ages to figure it out!

@fjebaker
Copy link
Owner

Awesome! Thank you Andy.

Just looking at why the CI is failing. test-binding line 84 is hitting the error

fitting: Error During Test at /home/runner/work/SpectralFitting.jl/SpectralFitting.jl/test/runtests.jl:86
  Got exception outside of a @test
  LoadError: type PowerLaw has no field K_1

Weird though because that method of freezing a parameter should be fine. Might be something else going on there, will quickly clone and see what I can find.

@fjebaker
Copy link
Owner

fjebaker commented Jun 26, 2024

Ah I see the problem. model1 is overwritten from the sum of two power laws to just a single power law component. I can quickly push a fix for this! You were quicker than me!

@phajy
Copy link
Contributor Author

phajy commented Jun 26, 2024

I've just fixed it. Classic problem of it working in the REPL when the commands were not executed in order.

@fjebaker
Copy link
Owner

Is this PR ready for review / merging? :)

@fjebaker fjebaker linked an issue Jun 26, 2024 that may be closed by this pull request
@phajy phajy marked this pull request as ready for review June 26, 2024 20:08
@fjebaker fjebaker merged commit c511239 into fjebaker:main Jun 26, 2024
1 check 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.

Parameter binding broken for frozen parameters
2 participants