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

Update theory.rst #664

Merged
merged 19 commits into from
Dec 13, 2023
Merged

Conversation

KulaginVladimir
Copy link
Collaborator

Proposed changes

This PR attempts to fill the Theory doc page, following #510. @RemDelaporteMathurin, @jhdark what do you think?

Types of changes

What types of changes does your code introduce to FESTIM?

  • 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)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e1d3b36) 98.91% compared to head (c28ea97) 98.91%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #664   +/-   ##
=======================================
  Coverage   98.91%   98.91%           
=======================================
  Files          57       57           
  Lines        2128     2128           
=======================================
  Hits         2105     2105           
  Misses         23       23           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KulaginVladimir KulaginVladimir marked this pull request as draft December 9, 2023 11:55
@KulaginVladimir
Copy link
Collaborator Author

Another point to discuss concerns splitting of this single file into several separate. Possibly, something like that can be done:

  • Theory.rst
    • Bulk physics.rst <- separate file
      • H transport
      • Soret effect
      • Conservation of chemical potential at interfaces
      • Heat transfer
    • Surface physics.rst <- separate file
      • Dirichlet BC
      • Plasma implantation approx (according to the work of @RemDelaporteMathurin)
      • Neumann BC
      • Robin BC

@RemDelaporteMathurin
Copy link
Collaborator

@gabriele-ferrero @SamueleMeschini feel free to make suggestions! You can see the compiled documentation by ReadTheDocs in the "checks" ok this PR

@RemDelaporteMathurin
Copy link
Collaborator

Another point to discuss concerns splitting of this single file into several separate.

If you really think the file is too long then we can split it in two. However, I would also like to keep the unique page for the Theory (where users can just scroll down instead of navigating to other pages).

  • Plasma implantation approx

Yes we need to detail this approximation in the documentation too.

@RemDelaporteMathurin RemDelaporteMathurin marked this pull request as ready for review December 11, 2023 16:54
@RemDelaporteMathurin RemDelaporteMathurin linked an issue Dec 11, 2023 that may be closed by this pull request
6 tasks
@KulaginVladimir
Copy link
Collaborator Author

If you really think the file is too long then we can split it in two. However, I would also like to keep the unique page for the Theory (where users can just scroll down instead of navigating to other pages).

One page is good

Yes we need to detail this approximation in the documentation too.

Ok

KulaginVladimir and others added 3 commits December 12, 2023 12:11
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
@RemDelaporteMathurin
Copy link
Collaborator

If we have a lot of references, is it worth using a proper bibliography management system like this?

Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

I noticed we never define what's an Arrhenius law. Maybe we can add something

docs/source/theory.rst Outdated Show resolved Hide resolved
docs/source/theory.rst Outdated Show resolved Hide resolved
KulaginVladimir and others added 2 commits December 12, 2023 16:34
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Comment on lines 21 to 23
trapping and a detrapping rate :math:`k_i` and :math:`p_i`, respectively, as well
as a trap density :math:`n_i`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RemDelaporteMathurin, possibly, we can add here something about the Arrhenius form, namely:

The diffusivity is usually assumed to follow the Arrhenius law of temperature [Reference?]: $D(T)=D_0e^{-\frac{E_d}{kT}}$, where $D_0$ is the diffusivity prefactor, $E_d$ - diffusion activation energy, $k$ - the Boltzmann constant, $T$ - temperature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could have something more general since it applies not only to the diffusivity but also to trapping rates, solubilities....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Therefore, a separate subsection somewhere? The first mention is in the Heat transfer section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly, this paper is appropriate for a reference. Original works of Arrhenius are in deutish as I see

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes maybe a separate subsection, no need for a reference

Copy link
Collaborator Author

@KulaginVladimir KulaginVladimir Dec 12, 2023

Choose a reason for hiding this comment

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

@RemDelaporteMathurin, I made an attempt

Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

Really nice work getting the bibliography to work @KulaginVladimir !! It's perfect!

I noticed the ToC (Table of Contents) has been flushed at the bottom of the page
image

It is supposed to be at the top right
image

I'll investigate this but I suspect it's somehow linked to the bibliography.
EDIT: maybe it's due to the size of the image... does this disappear if you reduce the size of the image using the width flag?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we don't need the abstracts here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To note, local build of docs works fine
1

@RemDelaporteMathurin
Copy link
Collaborator

@KulaginVladimir I still see the same behaviour in the ReadTheDocs compiled docs... do you see it too? https://festim--664.org.readthedocs.build/en/664/theory.html

@KulaginVladimir
Copy link
Collaborator Author

KulaginVladimir commented Dec 13, 2023

@RemDelaporteMathurin

@KulaginVladimir I still see the same behaviour in the ReadTheDocs compiled docs... do you see it too? https://festim--664.org.readthedocs.build/en/664/theory.html

Yes, unfortunately, I see it. By local doc build, I meant the html files that I build locally before submitting commits.

@RemDelaporteMathurin
Copy link
Collaborator

I've openened an issue executablebooks/sphinx-book-theme#792 to track the bibliography problem

Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

This is awesome @KulaginVladimir and had been needed for a very long time!

We'll also need this change in the fenicsx branch btw (without the conservation of chemical potential since it'll be handled differently).

We couldn't get rid of the bug of the ToC but it seems that once sphinx7 is supported by sphinx-book-theme then this will be fixed.

In the meantime, would you mind opening an issue here to remind us to have a look at this in a little while?

@RemDelaporteMathurin RemDelaporteMathurin merged commit d8a2063 into festim-dev:main Dec 13, 2023
4 checks passed
@KulaginVladimir KulaginVladimir deleted the Theory-docs branch December 13, 2023 09:56
@KulaginVladimir KulaginVladimir restored the Theory-docs branch December 13, 2023 10:15
@RemDelaporteMathurin RemDelaporteMathurin mentioned this pull request Dec 13, 2023
6 tasks
@KulaginVladimir KulaginVladimir deleted the Theory-docs branch December 18, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theory
2 participants