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

Onion: multilayered concentric sphere and ellipsoid shapes #333

Merged
merged 20 commits into from
Jul 2, 2024

Conversation

jeromefung
Copy link
Contributor

Description

Add shapes onion and onion_ell for multilayered concentric spheres and ellipsoids, respectively.

onion extends existing shapes coated and coated2 to allow for arbitrary number of concentric layers, not just 2 or 3. (The upper limit is set by MAX_NMAT.) Similarly, onion_ell extends existing ellipsoid shape to allow for an arbitrary number of concentric ellipsoidal layers. These new shapes might be useful for modeling particles whose refractive index varies continuously by approximating the variation stepwise.

Related issues

Fixes #293. (Confocal spheroids, briefly mentioned in #293, are not implemented.)

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the contributing guidelines
  • The new code complies with the existing code style.
  • The code compiles correctly in all relevant regimes (at least, make seq)
  • The new code does not rely on any Fortran or C++ sources or is disabled by NO_FORTRAN or NO_CPP preprocessor macros, respectively.
  • The change neither adds or removes files; otherwise, these changes are reflected in README.md in corresponding folders.
  • No warnings appear during debug compilation (at least, make seq OPTIONS=DEBUG, but better devtools/build_debug) or they are discussed below
  • Tests pass locally with my changes (at least, sh comp2exec seq in tests/2exec, but better devtools/test_new [seq]). If any errors appear, they are discussed below.
  • I have added tests that prove my fix is effective or that my feature works. And these tests pass. This includes new command line in suite files in tests/2exec (and potentially new ignore patterns). In some cases, it is desirable to add new tests to tests/equiv.
  • I have added/extended necessary documentation (if appropriate). If suggesting changes to the manual, I have used "Track changes" in the doc file.
  • I have looked through all changes introduced by this pull request (line by line), using git diff or, better, some GUI tool, to ensure that no unexpected changes are introduced.
  • Some tests in comp2exec seq fail, but same failures occur in master.
  • Descriptions of the new shapes have been added to the manual. However, I did not attempt to update Fig. 3 (pictures of predefined shapes) since it is already very crowded and a rearrangement would be needed to fit in additional shapes.

@myurkin myurkin added this to the 1.5 milestone Jun 16, 2024
@myurkin myurkin requested review from myurkin and removed request for myurkin June 16, 2024 11:44
@myurkin myurkin self-assigned this Jun 16, 2024
@myurkin myurkin self-requested a review June 16, 2024 11:45
Copy link
Member

@myurkin myurkin left a comment

Choose a reason for hiding this comment

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

Jerome, as I said before, your contribution is really comprehensive. Thanks for that! I will have some minor changes here and there (mostly in help strings and such, I will commit them later), but here are couple of questions, which you may want to address.

src/make_particle.c Outdated Show resolved Hide resolved
src/make_particle.c Outdated Show resolved Hide resolved
@jeromefung
Copy link
Contributor Author

Thanks very much -- I'll respond to your comments over the next day or two.

- adds several new functions
- moves common parts to a new file common.sh
- comparison of shape files now ignores differences in shape names (if it is expected)
- adds processing of exit codes and possibility to stop on the first error
- increases refractive indices, adds -grid 16, but balances this with -eps 1 (keeping the computational time approximately the same)
- adds several tests to onion.sh with zero-thickness shells
- now both tests run clean
…ated2)

- adds hints on inserting SVG files into MS Word in svg_images.txt
- Changes to the manual:
* polishes the description of new shapes (including some changes in variable names)
* specifies coated2 as deprecated shape
* updates the default value of MAX_NMAT to 60
* updates affiliation of the first author
* adds specific note about an interim version of the manual
- a bit cleaning in CMDIGNORE and OLDIGNORE
- makes test less stringent for "-int igt 3" (due to improvement of corresponding routines since 1.4.0)
- copies new test strings to other suites (haven't tested sparse, since it is all commented there)
…ons)

- MAX_N_SH_PARMS is now calculated directly from MAX_NMAT, but extra check for its sufficiency is added to PARSE_FUNC(shape)
- Use of MAX_NMAT is now more informative in various help strings (`-h m` and for new shapes) due to use of new macro TO_STRING(A)
- adds deprecated comments and warnings for shape coated2
- increments ADDA version to 1.5.0-alpha2 (as was done in the manual)
@myurkin
Copy link
Member

myurkin commented Jun 26, 2024

I have added various improvement. Feel free to comment on them (or add anything). Apart of that, we only need to finalize make_particle.c

@jeromefung
Copy link
Contributor Author

Your changes to files other than make_particle.c look good to me, and I think I've addressed your questions about make_particle.c. Please let me know if you have any other questions or concerns; I have no further issues that would preclude merging.

- tests of input parameters is now inclusive, so equality of any bounds (including that to 0 and 1) is allowed
- cosmetic improvements to description strings
- onion_r2 is now dynamically allocated
- adds function DescendingSearch - the implementation is still trivial, but the overall code is simpler
@myurkin
Copy link
Member

myurkin commented Jul 2, 2024

Jerome, I have added a few (cosmetic) improvements. Can you please quickly check that the latest version works on your side? And then I will merge.

@jeromefung
Copy link
Contributor Author

Looks good to me -- please go ahead and merge when you are ready.

@myurkin myurkin merged commit c609796 into adda-team:master Jul 2, 2024
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.

Multilayered sphere/ellipsoid
2 participants