ev_to_lambda is defined in two places:

1. PyTASER/TAS.py

   Line 24 in 953968c

   def ev_to_lambda(ev):

2. PyTASER/plotter.py

   Line 11 in 953968c

   def ev_to_lambda(ev):

You should remove one of the implementations and just import it in the other place.

Currently this repository doesn't have a LICENSE file defining how the project can be used. You should add one. I recommend an MIT license which is very permissive (allows others to use and share this package even in commercial software). However, GNU or BSD licenses are also a reasonable choice.

If you go for an MIT License, you can copy this file and just edit the name and date: https://github.com/utf/IFermi/blob/master/LICENSE

The Materials Project API key should be an optional argument with a default value of None here:

That's because, you can set the API in your ~/.pmgrc file and it will be used automatically so you don't have to provide it to the function. You can do that on your system by typing pmg config -a PMG_MAPI_KEY on the command line in an environment with pymatgen installed. You should change PMG_MAPI_KEY to your API key.

Can you also:

1. Rename api to api_key as api is not clear.
2. Remove your API key from all the examples. In general you don't want anyone else to know your API key as it can be used to impersonate yourself. You can make it the default value of None in the examples and add a comment to explain more. E.g.

api_key = None  # Use your API key here
GaAs_generator = generator.TASGenerator.from_mpid(
   "mp-2534", gaas_temp, gaas_conc, bandgap, api_key=api_key
)

The TAS topic is for tool-assisted speedrunning, not transient absorption spectroscopy. This tag should be removed from the repository.

It seems like you merged the jdos_simple and spin polarised functions. So now you can rename jdos_simple to just jdos.

There is a lot of commented out code saying something is broken: e.g.

bandgap_ev should be an optional argument with a default value of None, here:

If the band gap is not set then no line should be displayed.

Currently the most painful thing is working out which transitions you need to select when plotting. I think it would be best to add an "auto" mode, that automatically works out which transitions are the most important for the energy range of interest, and then automatically labels those.

You would enable this mode by setting relevant_transitions="auto" in the plotter as an alternative to passing a list.

And I think all of the examples, except one, should use "auto".

The Tas class doesn't need to store the energy mesh in both eV and wavelength as it is redundant information. E.g.

You should just keep the eV mesh and calculate the wavelength mesh when needed on the fly.

I found a random print statement:

Can you remove this and check for any others.

get_kpoint_weights or somewhere else in the code should give a proper error message if a line mode band structure is used instead of a calculation on a uniform k-point mesh.

This can be checked by looking if the band structure object is an instance of the BandStructureSymmLine class.

It would be beneficial for the docs to have a page on the theory behind pytaser, as well as a page on preferred settings/tips for new users.

The way to select specific spin transitions is very clunky, e.g.

Can you make it so that you just need "up" or "down" as "spin = down" is redundant.

We can't redistribute papers as they are copyright protected!

Currently, the bands are listed according to their energies - this makes it clunky for the user when deciding which bands to isolate in the spectrum.
Instead, we want to make the bands relative to the VB and CBs (e.g. if a band is 2 below VB (band -1), change it from band 7 to band -3. Likewise, 2 band above the CB (band +1) would be band +3 instead of band 12).

However, there are different VBs and CBs according to the spin and k-points, making it hard to identify one specific band number as the VB or 1 below the VB.

Fixing this issue will also simplify #14, as we can specify the "auto" mode as transitions associated with the 3 bands before and after the VB/CB.

There have been significant changes to the code since the last example plots, so it would be useful to make new example data for GaAs, Fe2O3 and TiO2. We could also test the high-quality data that Liam has, to allow for testing beyond the Materials Project.

The gaussian width argument is not used in the Generator.generate_tas function.

I think you just need to pass the value through to the jdos function (you will have to do this in multiple places).

Hi!
This is a really useful and well put-together package! 🙌 Excited to see how it develops!

I was trying to run this with CdTe, in the same way as the GaAs example notebook, and I'm running into this issue (see attachments). Basically it seems that the MP bandstructure has a down-sampled 12x12x12 mesh (I think), so manually hard-coding the mesh to 6x6x6 works, but the default doesn't.

I've pulled my hair out for a few hours now trying to figure out what was going on here & where it was failing 😅, so I think I've found mostly where the issue is coming from but not how best to fix. Hopefully those who are more familiar with the code will be able to spot/fix the issue quicker!

CdTe_TAS_Error_MWE.ipynb.zip

We need unit tests to check that the package is running properly and to prevent further changes breaking the code.

The modern way to write tests in python is using pytest

You can see an example of how tests should be setup by looking at the evgraf repo (although actually the test files should begin with the prefix test_): https://github.com/pmla/evgraf/tree/master/tests

If we want to submit to JOSS then having tests will be essential.

We need to create a documentation website or just have more details on how to run the code in the readme. An example of a package with a good readme is effmass

If we want to submit to JOSS then having good documentation will be essential.

I think the best approach for automatically selecting which transitions to plot is as follows:

1. Get the maximum absolute value of each transition within the plotting region.
2. Find the largest of all maximums.
3. Include transitions whose maximum value is within X% of largest maximum.

The X% should be an adjustable parameter, e.g., transition_cutoff. By default, I think a good value is 0.97 (e.g., 97%) but this could be tuned.

You mention abipy in the readme as a dependency and you also import it in your examples and library code. However, it doesn't seem that you actually use abipy anywhere?

If that is the case you can remove all the imports and update the readme.

Currently your python files are all in the root directory and cannot be installed as package.

The next step is to actually create a python package and also a setup.py file so pytaser can be pip installed.

You can do this in 3 steps.

1. Create a folder called pytaser and put the tas.py, generator.py and plotter.py in that folder. Also create an empty file called __init__.py in the folder too. This file lets python know you've created a package. Make the sure pytaser folder is lower case as all python packages shouldn't contain capital letters.
2. Update all the pytaser imports in your package and examples. E.g., import generator would become from pytaser import generator. Similarly, from tas import Tas would become from pytaser.tas import Tas. Make sure you do this in your examples too.
3. Create a setup.py file that will allow your package to be pip installable. This defines several things, like the dependencies of the package and some metadata. You can see an example setup.py file here. Note you won't need the extras_require and entry_points parts from that example.

Please merge #2 before making these changes.

Mac OS writes .DS_Store files for caching information about folders you visit in finder. Also PyCharm writes the .idea folder for storing information about the pycharm project.

Typically you don't want either of these in a git repo. The easiest way to handle them is using a .gitignore file in the root directory of the project which states which files to ignore.

To fix this you should:

1. Run find . -name .DS_Store -print0 | xargs -0 git rm -f --ignore-unmatch to remove .DS_Store files from git.
2. Run git rm -f .idea to remove the idea folder from git.
3. Add a .gitignore file to the root folder. You can use the one from IFermi: https://github.com/utf/IFermi/blob/master/.gitignore
4. Commit your changes to GitHub.

A lot of TAS spectra in literature use ΔT/T plots instead of just the ΔT plots that are currently used in PyTASER. To enable effective comparison against this data, we need to be able to switch the y-axis into a ΔT/T plot as well.

However, it is unclear if the denominator in ΔT/T is normalised according to JDOS_dark or JDOS_light - this seems to vary across literature. Furthermore, the denominator is often zero, despite a non-zero ΔT - this results in the ΔT/T shooting to infinity. While we could use a filter to ignore normalisation of T~0 values, there is no indicator of how this would affect the accuracy of the plot.
As such, we need a clear indication of how ΔT/T works.

Why are material_name, temp, and conc, needed in the plotter? E.g.

The docstring makes it sound like the temp will impact the plot but I don't think it should as the TAS has already been generated.

If these are just for additional labels and titles, then they should all be optional arguments with no default value (e.g., an empty string).

You can remove it from setup.py and the readme as you don't actually use it anywhere.