VirtualPhotonics / VTS

Virtual Tissue Simulator
https://virtualphotonics.org
Other
34 stars 9 forks source link

Feature/122 add ability to use layered concentric infinite cylinders with refractive index mismatch #133

Closed hayakawa16 closed 7 months ago

hayakawa16 commented 8 months ago

Hi, this is a draft of a PR so I can see file differences and make sure all looks good. I will assign reviewers when I'm ready for review.

hayakawa16 commented 8 months ago

I'm ready for review.

hayakawa16 commented 8 months ago

I just saw sonarcloud review. Give me a bit to address these.

hayakawa16 commented 7 months ago

Okay I'm ready for review.

hayakawa16 commented 7 months ago

Hi @lmalenfant, thanks for your review. I can make these edits.

hayakawa16 commented 7 months ago

I have updated code and pushed.

On another topic. I pulled master and ran an infile that I ran in August 2023 and obtained identical results. Then I pulled this branch and ran the same infile. The results are not identical. I think they are statistically equivalent but not sure. I'd like to know for sure before I merge this branch into master.

sonarcloud[bot] commented 7 months ago

Quality Gate Passed Quality Gate passed

Issues
2 New issues

Measures
0 Security Hotspots
85.7% Coverage on New Code
5.4% Duplication on New Code

See analysis details on SonarCloud

lmalenfant commented 7 months ago

I also noticed that we cannot merge right now because I made the master branch read only. We either have to force push or remove the lock. I checked that box so we can't push to master without a PR. I need to do more research to figure out the best protection.

hayakawa16 commented 7 months ago

I've performed my analysis of results using master and this branch and they agree within variance.

hayakawa16 commented 7 months ago

I feel okay about merging this into master. Should I check "Merge without waiting for requirements to be met (bypass branch protections)" and click "Merge pull request"?

lmalenfant commented 7 months ago

@hayakawa16 yes go ahead and merge, I think you just need to check the box and then the button will be available.