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.

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.

The Tas class doesn't need to store the energy mesh in both eV and wavelength as it is redundant information. E.g. https://github.com/WMD-group/PyTASER/blob/953968c3f0044e79644744415b33647dc7b2dae3/TAS.py#L60

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

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.

Why are material_name, temp, and conc, needed in the plotter? E.g. https://github.com/WMD-group/PyTASER/blob/953968c3f0044e79644744415b33647dc7b2dae3/plotter.py#L38

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).

Hi @savya10,

I tidied the package a little by:

• Running the black code formatter to format the file as standard python.
• Running the isort --profile black to sort the import statements.
• Removing unused import statements.
• Renamed the TAS.py file to tas.py as filenames shouldn't have capital letters in them.
• Renamed the tas class to Tas as python classes should always start with a capital letter (unlike functions).
• Reduced the length of some docstrings to keep the line length under 80 characters long.
• Removed documentation above imports or above functions (these aren't needed, as functions are documented using docstrings).

Most of these are just minor edits to for standard python formatting conventions (called PEP8 if you want to read more).

Once, you've looked through, please can you merge this PR and I will do a proper code review.

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.

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.

The gaussian width argument is not used in the Generator.generate_tas function. https://github.com/WMD-group/PyTASER/blob/953968c3f0044e79644744415b33647dc7b2dae3/generator.py#L508

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

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".

bandgap_ev should be an optional argument with a default value of None, here: https://github.com/WMD-group/PyTASER/blob/953968c3f0044e79644744415b33647dc7b2dae3/plotter.py#L37

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

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

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.

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.

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.