Add Python-based PDD solver option and fix sign convention inconsistencies #265
Conversation
…s before calculating
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #265 +/- ##
===========================================
+ Coverage 57.25% 59.47% +2.22%
===========================================
Files 104 108 +4
Lines 11690 12341 +651
===========================================
+ Hits 6693 7340 +647
- Misses 4997 5001 +4 ☔ View full report in Codecov by Sentry. |
|
@dalonsoa, I understand you probably don't have time to fully review this (and I need to add more tests, documentation, type annotations, and tidy things, so it is not ready to merge yet), but thought you might like to know: there will now be the option to use a drift-diffusion solver in Solcore which is very easy to install (100% Python), by interfacing with the Sesame solver. Definite improvements to be made still in how it is being used (e.g. mesh generation), but it seems to be performing quite well already! (Unfortunately, I also discovered an error while adding this functionality, where the current and voltage sign conventions were not consistent between the PDD and DA solvers, so that if you defined an n-on-p device with one PDD junction and a DA junction, the result would be incorrect. This will lead to backwards compatibility issues with voltage and current signs, but unfortunately I do not see a way around it which would not introduce any such issues :() |
pyproject.toml
Outdated
| @@ -39,6 +39,7 @@ dependencies = [ | |||
| "pyyaml", | |||
| "yabox", | |||
| "joblib", | |||
| "sesame@git+https://github.com/phoebe-p/sesame.git" | |||
There was a problem hiding this comment.
I'm not entirely sure, but I think that pypi do not allow dependencies that live in GitHub, only in pypi itself. I suspect that if Sesame is added this way, the deployment of Solcore will fail :( .
There was a problem hiding this comment.
I am actually having a hard time figuring out if this is possible (it definitely used to not be possible, but most of the threads I can find are a few years old and discussing setup.py projects, not pyproject.toml, so I wonder if anything has changed since then). It works in the build system on GitHub Actions and on my computer (by running pip install) but that does not mean that PyPI will accept it. I will make sure to test this if I can't figure it out by uploading to TestPyPI, and if it doesn't work find another solution.
There was a problem hiding this comment.
You were right that this is not allowed. I've uploaded the version of Sesame I was using to PyPI so it can be installed normally.
|
These a great news! The old Fortran PDD solver was a pain from day 1, so any efforts to replace it are good news. Having said that, the only reason there was a Fortran solver to start with was to tackle quantum wells and the convergence issues appearing with many layers with abruptly changing properties. If Sesame can solve that, fantastic! If not, it might be worth mentioning the different use cases each solver is useful for. In any case, I'll try to provide a review as soon as possible, hopefully by the end of the week. |
|
Yes, I am not planning to remove the Fortran solver or anything (now that the build system seems to be working well and on all platforms 😅). I've expanded the documentation a bit on some points of difference between the two solvers. |
|
Working on figuring out what is causing the tests to fail on 3.7 and 3.8. In the process of trying to figure this out, I discovered that there are no wheels for scipy for MacOS with M1/M2 chips for Python 3.7 or lower, so perhaps we should make the next release of Solcore for Python >= 3.8 instead of >= 3.7. |
- Refactor Sesame PDD into two files, update tests and documentation/docstrings - Remove changes to material_system causing recursion/stack overflow errors - add type annotations and docstrings. Black formatting - fix DA dark current overflow and relevant test - Improve automatic mesh for Sesame PDD - update documentation, add tests - make labelling of SRVs consistent between DA and Sesame. - remove GitHub reference for Sesame installation, refer to new PyPI version - Update outdated examples - Fix issue where plotting error causing examples in GitHub actions to fail - Change build for S4 to install setuptools explicitly (necessary in Python 3.12) - Sesame now scans from long to short wavelengths in QE calculations to improve changes of convergence, and skips wavelengths where absorption = 0 - Fix issue where Fortran PDD did not work if all voltages < 0 - Upgrade from devpy to spin (renamed), and to latest version of meson in the build system
|
@dalonsoa, I need to merge this PR (Ned and I are running a workshop at UNSW this week where we'd like to use the new Sesame solver 😀). Thank you for pointing out the issue with Github dependencies, that would have been annoying to have to solve last-minute! Of course if you have any other feedback later I would be happy to hear it! |
No description provided.