Green's function for da solver #207
Conversation
| @@ -383,6 +395,52 @@ def bc(ya, yb): | |||
| return out | |||
|
|
|||
|
|
|||
| def get_J_sc_diffusion_green(xa, xb, g, D, L, y0, S, ph, side='top'): | |||
There was a problem hiding this comment.
I know that solcore is not that great with docstrings - to put it mildly - but it will be great if any new addition were thorough with that and type annotations, to make the code more readable. Nothing to do in this first pass, but something to tkae into account.
There was a problem hiding this comment.
Docstring has been added, similar to the other functions. Do you think the newly introduced top level functions need a docstring (introduced because of your third comment)? I am not sure what you mean with type annotations.
There was a problem hiding this comment.
Ideally, yes, so the functions are really standalone. About type annotations, you have a very nice blog post describing the matter here: https://towardsdatascience.com/type-annotations-in-python-d90990b172dc
| cadd = -2. * S / D * y0 / (1. - crvel) * np.exp(-xbL) | ||
| cp = 1. | ||
|
|
||
| def fun(x): |
There was a problem hiding this comment.
While I've used them, I am not a big fan of internal functions like this one (and the one below) because they cannot be tested. I would suggest you define them as top level functions, passing all the relevant arguments to them.
Again, the point being improving the overall quality of solcore's code
There was a problem hiding this comment.
I agree. Quad_vec does not support the "arg" argument like the other functions of the scipy.integate package (although the feature has been requested and will be implemented in the future). My suggestion is using the partial method from the functiontool package.
Codecov Report
@@ Coverage Diff @@
## develop #207 +/- ##
===========================================
+ Coverage 45.47% 45.84% +0.37%
===========================================
Files 84 84
Lines 9029 9091 +62
===========================================
+ Hits 4106 4168 +62
Misses 4923 4923
Continue to review full report at Codecov.
|
|
Hi Diego, Thanks very much for taking the time to review the pull request and for your comments. I agree with all of the comments, see above. Regarding the limitation of this method. From the theoretical point, it of course inherits the limitations of the depletion approximation. Numerically, the one that I have encountered is that the solution behaves poorly as the junction width becomes way larger the diffusion length (e.g. for junction with a lot of defects). In my code, I am guarding against this by assuming that if x>1.e2 then sinh(x) = cosh(x) = .5*e^x (for numpy this is actually the case). Keep in mind that this problem also affects the solve_bvp method, as the differential equation becomes stiffer and the accuracy drops, and the Best, |
There was a problem hiding this comment.
Using partial is a great way of dealing with that issue. Well done! Keeping aside - optionally - completing the docstrings for the extra top level functions and adding type annotations, I think this is pretty much ready to:
- write tests
- update documentation
Great work! I believe this first addition of functionality to Solcore in a long, long time! Let's hope more people follow your lead!
| @@ -383,6 +395,52 @@ def bc(ya, yb): | |||
| return out | |||
|
|
|||
|
|
|||
| def get_J_sc_diffusion_green(xa, xb, g, D, L, y0, S, ph, side='top'): | |||
There was a problem hiding this comment.
Ideally, yes, so the functions are really standalone. About type annotations, you have a very nice blog post describing the matter here: https://towardsdatascience.com/type-annotations-in-python-d90990b172dc
|
Hi Diego Docstrings, tests and documentation has been updated. My system passes all the new tests and I get a 100% coverage for the added code. Regarding the type annotations, I am facing some problems though. The examples from the article you sent me do not produce any warning when there is a type mismatch in the function arguments. The article hints that type evaluations have been postponed. This makes the implementation vague (how am I supposed to test), so I skipped them. Best, |
vathomass
changed the title
|
Many thanks for implenting this. It looks great! I'm going to merge it as it is now as it ticks all the boxes. To me, type annotations are more like documentation, to guide the user and the developer towards consistent, correct code. Most IDEs like PyCharm and VSCode can be configured to check the consistenncy of the types in the background - producing a warning if there is something wrong - or you can use an static type checker like |
Hi,
This is the 1st draft implementation for the semi-analytic solution for the DA solver, as discussed in #205.
For my testing, I am using the
MJ_solar_cell_using_DA.pyfrom the examples directory, where one can switch between the original and the new implementation by passing the option'da_mode': 'bvp'or'da_mode': 'green'along with the other options to the solar cell solver.For now, I have not implemented any tests, neither have I updated the documentation.
I welcome any feedback you may have.
Best,
Thomas