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

Use scattmap and model to inject soureces #314

Merged
merged 5 commits into from
Apr 4, 2025

Conversation

Yong2Sheng
Copy link
Contributor

Updated source_injector module and tutorial notebooks to adapt to DC3:

  1. It uses scattmap to inject point sources when using detector response.
  2. Now users can inject a model containing multiple sources.

@Yong2Sheng
Copy link
Contributor Author

Hi @israelmcmc, it seems that I have the same issue as Krishna had in his PR. My source_injector branch is based on the develop branch from the cosipy repo. When merging into v0.3.x branch, there are changes that I didn't make (see the history).

@israelmcmc
Copy link
Collaborator

@Yong2Sheng I see. It seems that the issue is then caused by branching from develop and trying to merge into v0.3.0? Can you please try rebasing your branch and opening a new PR? You can do this with:

git rebase --onto v0.3.0 62af6b0 source_injector

62af6b0 is the last commit in develop at the time you branch out (my commit above that says "Merge branch 'v0.3.x' into develop"). This command applies all commits in the source_injector branch since then onto v0.3.0. See the --onto documentation here: https://git-scm.com/docs/git-rebase


if response_frame == "local" or response_frame == "galactic":
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't want to keep this check for either spacecraftframe or galatic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should keep it. I will bring it back in my next push with unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

should this file be removed ? It looks like a personal test with personal path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It came from other's commits. This is fixed now after rebasing my PR.

@Yong2Sheng
Copy link
Contributor Author

Hi @GallegoSav, I am trying merge into v0.3.x, but my source injector is based on the develop branch, so it contains some commit history I didn't do. I will rebase the source injector branch to v0.3.x and open a new PR for this.

@GallegoSav
Copy link
Contributor

Hi @GallegoSav, I am trying merge into v0.3.x, but my source injector is based on the develop branch, so it contains some commit history I didn't do. I will rebase the source injector branch to v0.3.x and open a new PR for this.

Hi @Yong2Sheng , is this new pull request opened ?

@Yong2Sheng
Copy link
Contributor Author

Hi @GallegoSav, I rebased this branch and it seems the rebase is propagated into this PR, so I don't need to open a new PR. You can continue the review from here. The test unit issue will be fixed after PR287 is merged into this PR.
Thanks!

@israelmcmc
Copy link
Collaborator

@Yong2Sheng #287 is now merged. Please check again if this fixes your unit tests.

@Yong2Sheng
Copy link
Contributor Author

@israelmcmc Thank you! I will test it.

@Yong2Sheng
Copy link
Contributor Author

Hi @GallegoSav, the unit test has been fixed.

@GallegoSav GallegoSav merged commit 22494c6 into cositools:v0.3.x Apr 4, 2025
2 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.

None yet

3 participants