qpv-research-group / solcore5

A multi-scale, python-based library for the modelling of solar cells and semiconductor materials
https://www.solcore.solar/
Other
133 stars 77 forks source link

Add Python-based PDD solver option and fix sign convention inconsistencies #265

Closed phoebe-p closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (31d9110) 57.25% compared to head (692c986) 59.47%.

Files Patch % Lines
solcore/sesame_drift_diffusion/solve_pdd.py 93.10% 10 Missing :warning:
solcore/solar_cell_solver.py 45.45% 6 Missing :warning:
...olcore/sesame_drift_diffusion/process_structure.py 98.32% 3 Missing :warning:
...re/analytic_solar_cells/depletion_approximation.py 84.61% 2 Missing :warning:
solcore/material_data/mobility.py 0.00% 1 Missing :warning:
...poisson_drift_diffusion/DriftDiffusionUtilities.py 85.71% 1 Missing :warning:
solcore/poisson_drift_diffusion/__init__.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

phoebe-p commented 1 year ago

@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 :()

dalonsoa commented 1 year ago

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.

phoebe-p commented 1 year ago

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.

phoebe-p commented 1 year ago

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.

phoebe-p commented 1 year ago

@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!