qpv-research-group / solcore5

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

PDD solver: error in the documentation on how to access the output #109

Open dalonsoa opened 4 years ago

dalonsoa commented 4 years ago

The documentation for the PDD solver states that the output is provided as a nested dictionary. However, that is not correct: it is provided as a State object with the different keys accesible as attributes. For example:

solar_cell[junction_index]['QE']['EQE']

should be:

solar_cell[junction_index].qe.EQE

The examples are incorrect and so is the description of the dictionary at the end.

Abelarm commented 2 years ago

I could also take care of this :)

dalonsoa commented 2 years ago

Please, go ahead! Please, let me know if after making these changes, something is still not working or is not accurate.

Abelarm commented 2 years ago

I am having a problem with the PDD, I have installed everything with the “with_pdd” no problem but keep having the warning about the ddModel could not be imported

dalonsoa commented 2 years ago

Have a look at this https://github.com/qpv-research-group/solcore5/issues/168#issuecomment-916032877

It should be a solved problem in the current master and develop branches. It might be you need to update?

Abelarm commented 2 years ago

I already have that version the point is that I do not have file "ddModel.cp39-win_amd64.pyd"

dalonsoa commented 2 years ago

That's funny. It does work with Windows and Python 3.9 in the CI system...

Could you run f2py manually in the poisson_drift_diffusion folder to compile the fortran library? Or maybe uninstall and then re-install again Solcore in edit mode but maybe commenting out this line, so you see if there is any error.

Abelarm commented 2 years ago

Great I run this command: python -m numpy.f2py DDmodel-current.f95-m ddModel -h ddModel.pyf

and it created a .so and a .pyf and now it works!!

Abelarm commented 2 years ago

I am not really sure what to fix here:

1 All the examples apart the last one works fine.

2 For the second example the Bandstructure i cannot access as a State (is a dict in-fact) image

3 For the second example I was able to change my_solar_cell[0].recombination_currents['Jrad'] to my_solar_cell[0].recombination_currents.Jrad (as example) and it works

3b For the second part of the example I get an error for this line, I would change it be be a f-string instead of format

plt.text(0, 200, 'Voc = {:4.1f} V\n'
                 'Isc = {:4.1f} A/m${^2}$\n'
                 'FF = {:4.1f} %\n'
                 'Pmpp = {:4.1f} W/m${^2}$'.format(my_solar_cell.iv['Voc'], my_solar_cell.iv['Isc'],
                                           my_solar_cell.iv['FF'] * 100, my_solar_cell.iv['Pmpp']))

4 For the last example tries to access: my_solar_cell[0].qe_data.wavelengths which does not exist in the State even when changing to my_solar_cell[0].qe.wavelengths

So it's a mix of several things here :/

dalonsoa commented 2 years ago

I'm sorry it took me so long to answer. Yes, this is really disconcerting and, honestly, very confusing for the users being the output so inconsistent.

I think that the only solution - and is not really a solution - that does not break backwards compatibility is to go through all the relevant functions in the PDD solver and document exactly what their outputs are, event if these are weird and inconsistent.

Eventually, we need to put some order here and refactor all the outputs, so things make sense, but I don't think we are there, yet, I am afraid.