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

Yuhsuan/150 fixed fitting #1183

Merged
merged 6 commits into from
Oct 3, 2022
Merged

Yuhsuan/150 fixed fitting #1183

merged 6 commits into from
Oct 3, 2022

Conversation

YuHsuan-Hwang
Copy link
Contributor

@YuHsuan-Hwang YuHsuan-Hwang commented Aug 22, 2022

Description
This PR is for the second item of #150: fixing initial values

  • SetInitialValues() checks the booleans sent from frontend and stores the fitting value vector index of the Gaussian component parameters infit_values_indexes. -1 is stored if the parameter is fixed, indicating that the initial value should be used instead.
  • GetGaussianParams() checks fit_values_indexes and returns the values that should be used for a Gaussian component.

Tested that fitting with no parameters fixed remains the same result.

corresponding frontend PR: CARTAvis/carta-frontend#1984

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / added corresponding fix
  • protobuf updated to the latest dev commit / no protobuf update needed
  • added reviewers and assignee
  • added ZenHub estimate, milestone, and release

@kswang1029
Copy link
Contributor

a quick comment: for the center position, if it is fixed, we dont need to show the '(arcsec)' string
Center X = 6:12:54.3777367749 (arcsec) (fixed)
Center Y = 17:59:14.1574465497 (arcsec) (fixed)

@kswang1029
Copy link
Contributor

@YuHsuan-Hwang a quick question: if we have two gaussian components, with the current implementation, we cannot have one gaussian with all parameters fixed. Is this intended? Can we make it more flexible so as long as there is one parameter free regardless how many components are, we can tigger the image fit?

@YuHsuan-Hwang
Copy link
Contributor Author

a quick comment: for the center position, if it is fixed, we dont need to show the '(arcsec)' string Center X = 6:12:54.3777367749 (arcsec) (fixed) Center Y = 17:59:14.1574465497 (arcsec) (fixed)

@YuHsuan-Hwang a quick question: if we have two gaussian components, with the current implementation, we cannot have one gaussian with all parameters fixed. Is this intended? Can we make it more flexible so as long as there is one parameter free regardless how many components are, we can tigger the image fit?

Yes, I should have make it more flexible. These two are frontend issues; fixed in the frontend PR.

Copy link
Contributor

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

this looks good. all e2e tests are passing. 👍

@confluence confluence merged commit 638d36d into dev Oct 3, 2022
@confluence confluence deleted the yuhsuan/150_fixed_fitting branch October 3, 2022 08:18
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.

None yet

5 participants