Skip to content

Conversation

@rozyczko
Copy link
Member

As requested and described by @jfkcooper this is the library implementation of bilayers in EasyReflectometryLib.

Please scrutinize docs\tutorials\simulation\bilayer.ipynb for correctness and API.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: ['chore', 'fix', 'bugfix', 'bug', 'enhancement', 'feature', 'dependencies', 'documentation']

@rozyczko rozyczko requested a review from jfkcooper January 31, 2026 14:02
@rozyczko rozyczko added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] high Should be prioritized soon labels Jan 31, 2026
@rozyczko rozyczko mentioned this pull request Jan 31, 2026
@codecov
Copy link

codecov bot commented Jan 31, 2026

Codecov Report

❌ Patch coverage is 91.26984% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.29%. Comparing base (205db70) to head (4333c43).

Files with missing lines Patch % Lines
src/easyreflectometry/sample/assemblies/bilayer.py 91.20% 0 Missing and 11 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           append_sample     #292      +/-   ##
=================================================
+ Coverage          88.10%   88.29%   +0.19%     
=================================================
  Files                 45       46       +1     
  Lines               2471     2597     +126     
  Branches             288      307      +19     
=================================================
+ Hits                2177     2293     +116     
+ Misses               233      232       -1     
- Partials              61       72      +11     
Flag Coverage Δ
unittests 88.29% <91.26%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/easyreflectometry/sample/__init__.py 100.00% <100.00%> (ø)
src/easyreflectometry/sample/assemblies/bilayer.py 91.20% <91.20%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

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

Basically only one comment on the code parts, and easily rectified. I will run the notebook now and give you feedback on this, but doing it in github is not fun for notebooks, so I submit these comments now

tail_layer = LayerAreaPerMolecule(
molecular_formula='C32D64',
thickness=16.0,
solvent=air,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a lipid monolayer having the solvent be air I think is correct, but for the bilayer there is no air in the system, so a default of D2O make more sense

back_tail_layer = LayerAreaPerMolecule(
molecular_formula=tail_layer.molecular_formula,
thickness=tail_layer.thickness.value,
solvent=air_back,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, change the air to D2O as default

# Create layer collection: front_head, front_tail, back_tail, back_head
bilayer_layers = LayerCollection(
front_head_layer,
tail_layer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the naming here would be more consistent with 'front_tail_layer' instead of 'tail_layer'

def test_custom_layers(self):
"""Test creation with custom head/tail layers."""
d2o = Material(sld=6.36, isld=0, name='D2O')
air = Material(sld=0, isld=0, name='Air')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Testing with air here doesn't make so much sense, since there is no air in the system. I know it is just for testing, and doesn't matter, but you could change the name to 'air_matched_water' and it would be consistent with the setup (changing nothing else)

"""Test serialization/deserialization round trip."""
# When
d2o = Material(sld=6.36, isld=0, name='D2O')
air = Material(sld=0, isld=0, name='Air')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar comment about air to others. change it to air matched water if you want

@jfkcooper
Copy link
Collaborator

Notes from the notebook:

  • Depending on what the aim for the notebook is, I would remove all of the cells doing checking an printing, as they would not be interesting to a user - it would be assumed that they just work.

  • The superphase for the complete sample should be silicon. The structure should be:
    Si | SiO2 | head | tail | tail | head | water it should look something like this:

image
  • For the tail groups, I believe you have the SLD for a deuterated version of the tails, but this is not the 'normal' configuration, and I think for an exemplar notebook it is better to use the hydrogenous version.

  • For the asymmetric hydration, it isn't clear where but there seems to be a bug or something, as you would expect a difference in the curves. I would highlight the change by showing the SLD curves rather than just the reflectivity though, as this is where you can know that it is being done correctly

@jfkcooper
Copy link
Collaborator

Thinking on it, I would also use this as a way to demonstrate mutliple contrasts for the lipid model, so using H2O as the solvent and subphase as well as D2O, and then showing the SLD curves, and the reflectivity curves (and the ability to set the constraints between the two models (or however you would put the models together).

The most common use case of the lipid bilayer would be for fitting mulitple contrast (water) data.

@rozyczko
Copy link
Member Author

rozyczko commented Feb 2, 2026

Addressed the code issues and modified the notebook to reflect Jos' comments.

@rozyczko rozyczko requested a review from jfkcooper February 2, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[priority] high Should be prioritized soon [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants