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.

Sign up for GitHub

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

Jump to bottom

Overlap #565

Merged
merged 2 commits into from
May 24, 2022
Merged

Overlap #565

merged 2 commits into from
May 24, 2022

Conversation

martinkilbinger
Copy link
Contributor

@martinkilbinger martinkilbinger commented Apr 19, 2022 •
edited by lbaumo
Loading

Summary

Remove code to check and flag overlapping regions. This code use distances to neighbouring tile centers, and for some reason did not remove all overlapping objects. This obsolete code is now superceded by simple RA/Dec cuts in sp_validation.

Reviewer Checklist

Reviewers should tick the following boxes before approving and merging the PR.

  • The PR targets the develop branch
  • The PR is assigned to the developer
  • The PR has appropriate labels
  • The PR is included in appropriate projects and/or milestones
  • The PR includes a clear description of the proposed changes
  • If the PR addresses an open issue the description includes "closes #"
  • The code and documentation style match the current standards
  • Documentation has been added/updated consistently with the code
  • All CI tests are passing
  • API docs have been built and checked at least once (if relevant)
  • All changed files have been checked and comments provided to the developer
  • All of the reviewer's comments have been satisfactorily addressed by the developer

sfarrens and others added 2 commits April 1, 2022 18:05
@sfarrens @martinkilbinger
Final clean up (CosmoStat#557)
689d3fb 
* added CD GitHub action

* fixed bug in RunLog class (CosmoStat#548)

* 800deg2 (CosmoStat#544)

* added config files for matching 800deg2 survey

* Added Xavier as contributor; match external catalogue fix

* match 800

* match_ext: output distance

* match_ext output_dist def value

* Tutorial documentation (CosmoStat#543)

* updating job script and tutorial

* tutorial; fix config file

* tutorial finished

* no change

* psfex update (minor bug)

* psfex running

* updated MCCD config files

* comment in config file

* config file fix for MCCD

* config file fix for MCCD

* removed unused option in usage str

* Update docs/source/pipeline_tutorial.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* improved tutorial according to PR review

* Update docs/source/pipeline_tutorial.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* tutorial clean-up

* post_proc cleaned up; tutorial added output dir names

* changes to tutorial, post-proc and mccd plot doc

* get_images error msg testing

* Update docs/source/pipeline_tutorial.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* Update docs/source/pipeline_tutorial.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* Update docs/source/pipeline_tutorial.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* Update docs/source/pipeline_tutorial.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* removed prompt from bash example

* Update docs/source/post_processing.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* PR review changes to doc

* PR review changes to doc

* Update docs/source/post_processing.md

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>

* added contribution guidelines (CosmoStat#550)

* added contribution guidelines

* Update CODE_OF_CONDUCT.md

* tutorial bugfixes (CosmoStat#552)

Co-authored-by: Lucie Baumont <lucie.baumont@cea.fr>

* removed pep8 error exceptions (CosmoStat#554)

* Understanding the API docs (CosmoStat#553)

* added md for understanding the api docs

* updates following reviwer comments

* Update package versions (CosmoStat#539)

* updated doc-tests

* updated package versions

* pin documentation packages

* updated ShapePipe version and cleaned up

* Config file clean up (CosmoStat#556)

* Removed unused config files in example/cfis

* Removed obsolete config dirs tutorial and ps3pi_cfis

* removed old unions config files

* Update docs/source/configuration.md

Co-authored-by: Martin Kilbinger <martin.kilbinger@cea.fr>

* final clean up

Co-authored-by: Martin Kilbinger <martin.kilbinger@cea.fr>
Co-authored-by: Lucie Baumont <lucie.baumont@stonybrook.edu>
Co-authored-by: Lucie Baumont <lucie.baumont@cea.fr>
@martinkilbinger
removed obsolete distance-based overlap region flagging
72d6bb8
@martinkilbinger martinkilbinger requested a review from lbaumo April 19, 2022 14:49
@martinkilbinger martinkilbinger self-assigned this Apr 19, 2022
@sfarrens
Copy link
Member

sfarrens commented May 4, 2022

@martinkilbinger I had a look and I believe the reason GitHub is showing the wrong number of changed files is because the commits to the master branch have been squashed.

See this comment from Stack Overflow.

This happens with GitHub when you squash commits merged in from the target branch.

I had been using squash and merge with Github as the default merge strategy, including merges from the target branch. This introduces a new commit and GitHub doesn't recognize that this squashed commit is the same as the ones already in master (but with different hashes). Git handles it properly but you see all the changes again in GitHub, making it annoying to review. The solution is to do a regular merge of these pulled in upstream commits instead of a squash and merge. When you want to merge in another branch into yours as a dependency, git merge --squash and revert that single commit before pulling from master once that other branch has actually made it to master.

I think the final merge should be unaffected by this.

@lbaumo you can more easily review this PR by only looking at the changed files here.

@martinkilbinger martinkilbinger merged commit 9080701 into CosmoStat:develop May 24, 2022
@martinkilbinger martinkilbinger deleted the overlap branch May 24, 2022 11:15
@martinkilbinger martinkilbinger restored the overlap branch May 30, 2022 12:44
@sfarrens sfarrens mentioned this pull request Sep 16, 2022
Merged
@martinkilbinger martinkilbinger deleted the overlap branch November 13, 2023 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbaumo lbaumo lbaumo approved these changes

Assignees

@martinkilbinger martinkilbinger

Labels
clean up
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@martinkilbinger @sfarrens @lbaumo