Add support for non-diagonal supercells

[RastislavTuranyi]

As discussed, and mentioned in #310, I had a try at adding support for non-diagonal supercells to phonon calculations. The Python API change is small: simply adding support for lists of length 9, which are then turned into a 3x3 matrix. The CLI API is much trickier due to the restrictions that typer/click impose (certain hacks notwithstanding); for now, I and @ajjackson decided to require the supercell input to be a whitespace-separated string, e.g. "1 2 3"), which would have to be passed in as a string. The other option we considered was having two different options for the two cases, e.g. --supercell-diagonal and --supercell-nondiagonal. However, we aren't sure what would be the best approach; what do you think?

Also, I have tried adding some tests, but let me know if more/different tests are required.

[ElliottKasoar]

We'll need to figure out how this fits with #307. If that's merged, supercell would be be a tuple (--supercell x y z), but I don't think this works if we want a input variable length, so I may need to revert that change.

[RastislavTuranyi]

We'll need to figure out how this fits with #307. If that's merged, supercell would be be a tuple (--supercell x y z), but I don't think this works if we want a input variable length, so I may need to revert that change.

In that case, we could go for the other option of having two different options, --supercell-diagonal 1 2 3 and --supercell-nondiagoal 1 2 3 4 5 6 7 8 9, if that would be preferable? We weren't sure which approach would work better with the API you are aiming for

[ElliottKasoar]

We'll need to figure out how this fits with #307. If that's merged, supercell would be be a tuple (--supercell x y z), but I don't think this works if we want a input variable length, so I may need to revert that change.

In that case, we could go for the other option of having two different options, --supercell-diagonal 1 2 3 and --supercell-nondiagoal 1 2 3 4 5 6 7 8 9, if that would be preferable? We weren't sure which approach would work better with the API you are aiming for

I'm not set on anything too specific, but yes given that the approach consistent with the current main would be "1x2x3x4x5x6x7x8x9", which is a bit unwieldy, separating the options might make most sense.

I'm hesitant to change --supercell though. Do you think it would still make sense to have --supercell and --supercell-nondiag?

If you can find alternatives that let us neatly unify them (maybe with a custom type - I've seen that as a suggestion for click Unions, which typer is based on) that would also be welcome.

[ajjackson]

I'm not set on anything too specific, but yes given that the approach consistent with the current main would be "1x2x3x4x5x6x7x8x9", which is a bit unwieldy, separating the options might make most sense.

It isn't only unwieldy; it's nonsensical! In the diagonal case we can "multiply" to describe the overall cell volume, but it doesn't work for matrix elements.

How about --supercell and --supercell-matrix?

It looks like Typer is also rubbish at mutually-exclusive arguments 😞
fastapi/typer#140

[RastislavTuranyi]

Another option I thought of would be to have only --supercell, but make it be able to be called multiple times, so --supercell 1 2 3 would be diagonal, and --supercell 1 2 3 --supercell 4 5 6 --supercell 7 8 9 would be the full matrix. That said, though, I am not sure I like it, and from some quick text I have a feeling it would not necessarily be easy to implement with typer. Overall, it seems to me that the --supercell and --supercell-matrix might be the best option.

If you can find alternatives that let us neatly unify them (maybe with a custom type - I've seen that as a suggestion for pallets/click#1729, which typer is based on) that would also be welcome.

I had a try experimenting with this, but I can only see how that would allow us to parse the two strings, i.e. --supercell "1 2 3" vs --supercell "1 2 3 4 5 6 7 8 9"; I can't figure out how that could be done for the tuple case, but then I am not experienced with typer/click so there might be a way. What I have tried is to use the click_type=Union() parameter, but that leads to only the first value being passed in to the custom Union type, i.e. --supercell 1 2 3 -> the Union.convert() method only receives the 1.

I have also tried the hack I mentioned earlier, but I'm unclear on how to make it work with typer since it's unclear where/how typer actually uses click.Option, and typer doesn't have any API documentation

There is probably a way to use both of the above workarounds, but I reckon it will probably need much deeper understanding of how typer works under the hood.

Overall, it seems to me that the --supercell and --supercell-matrix might be the best option.

[oerc0122]

Another option I thought of would be to have only --supercell, but make it be able to be called multiple times, so --supercell 1 2 3 would be diagonal, and --supercell 1 2 3 --supercell 4 5 6 --supercell 7 8 9 would be the full matrix. That said, though, I am not sure I like it, and from some quick text I have a feeling it would not necessarily be easy to implement with typer. Overall, it seems to me that the --supercell and --supercell-matrix might be the best option.

Single responsibility principle in me is screaming at this suggestion. They're two different jobs which have two different outcomes and two different meanings. They shouldn't share a name even if they ultimately affect the same var.

[ajjackson]

I think this might be exactly how Typer wants to do it (with a hint like list[tuple[int, int, int]]?), but not how humans want to do it 😅

[ElliottKasoar]

Another option I thought of would be to have only --supercell, but make it be able to be called multiple times, so --supercell 1 2 3 would be diagonal, and --supercell 1 2 3 --supercell 4 5 6 --supercell 7 8 9 would be the full matrix. That said, though, I am not sure I like it, and from some quick text I have a feeling it would not necessarily be easy to implement with typer. Overall, it seems to me that the --supercell and --supercell-matrix might be the best option.

Single responsibility principle in me is screaming at this suggestion. They're two different jobs which have two different outcomes and two different meanings. They shouldn't share a name even if they ultimately affect the same var.

Is your objection to using --supercell in principle for both diagonal terms (if three values are specified) and the full matrix (if nine are specified), which doesn't seem unreasonable in theory, or that the meaning of the first --supercell x y z changes, which to me is more problematic?

[ElliottKasoar]

Also note @RastislavTuranyi you'll need to rebase to include changes from #307

[oerc0122]

Another option I thought of would be to have only --supercell, but make it be able to be called multiple times, so --supercell 1 2 3 would be diagonal, and --supercell 1 2 3 --supercell 4 5 6 --supercell 7 8 9 would be the full matrix. That said, though, I am not sure I like it, and from some quick text I have a feeling it would not necessarily be easy to implement with typer. Overall, it seems to me that the --supercell and --supercell-matrix might be the best option.

Single responsibility principle in me is screaming at this suggestion. They're two different jobs which have two different outcomes and two different meanings. They shouldn't share a name even if they ultimately affect the same var.

Is your objection to using --supercell in principle for both diagonal terms (if three values are specified) and the full matrix (if nine are specified), which doesn't seem unreasonable in theory, or that the meaning of the first --supercell x y z changes, which to me is more problematic?

That the meaning of supercell is changing depending on how many times it's called. Notably with "the same arguments".

You could be fooled into interpreting it is "ah, we do the computation sequentially with 3 supercells, the first diag(1, 2, 3), the second with diag(4, 5, 6) the third with diag(7, 8, 9)" Which depending on the values of 1-9 may not be reasonable. (3, 1, 1), (1, 3, 1), (1, 1, 3) supercell in x, then y, then z and get the appropriate phonon blocks out. OR supercell 3 in the cardinal directions with some off diagonal.

[ElliottKasoar]

Another option I thought of would be to have only --supercell, but make it be able to be called multiple times, so --supercell 1 2 3 would be diagonal, and --supercell 1 2 3 --supercell 4 5 6 --supercell 7 8 9 would be the full matrix. That said, though, I am not sure I like it, and from some quick text I have a feeling it would not necessarily be easy to implement with typer. Overall, it seems to me that the --supercell and --supercell-matrix might be the best option.

Single responsibility principle in me is screaming at this suggestion. They're two different jobs which have two different outcomes and two different meanings. They shouldn't share a name even if they ultimately affect the same var.

Is your objection to using --supercell in principle for both diagonal terms (if three values are specified) and the full matrix (if nine are specified), which doesn't seem unreasonable in theory, or that the meaning of the first --supercell x y z changes, which to me is more problematic?

That the meaning of supercell is changing depending on how many times it's called. Notably with "the same arguments"

I wouldn't quite describe it as being called, it's the typer way of creating lists, so [(x, y, z)] being different to [(a, b, c), (d, e, f), (g, h, i)] isn't quite so unreasonable, but yes it probably is still more confusing than alternatives, given that you might start with --supercell x y z and want to add off diagonal terms.

[ajjackson]

I agree that both

• within Typer's way of thinking it makes sense that it would form an array
• to a reasonable person, it looks more like you are declaring multiple job steps

[RastislavTuranyi]

Just found this typer PR which does almost what we want, jut requiring a specific separator, but it's likely going to be a while until that gets into an official release.

Also found this hack which actually pre-processes the CLI and therefore seems to be the only thing that has a chance of working with typer.

EDIT: The pre-processing hack works for positive integers, but it can't deal with negative values (e.g. 1 1 0 -1 1 0 0 0 2) as is.

[ElliottKasoar]

One option which I'm pretty sure works if duplicating the custom TyperDict class and parsers we already have, which then allows us to parse arbitrary tuples e.g. --supercell "(2, 2, 2)".

E.g.

def parse_tuple_class(value: tuple):
    """
    Convert string input into a tuple.

    Parameters
    ----------
    value : str
        String representing tuple to be parsed.

    Returns
    -------
    TyperTuple
        Parsed string as a tuple.
    """
    if isinstance(value, tuple):
        return TyperTuple(value)
    return TyperTuple(ast.literal_eval(value))

I think it's marginally nicer as a tuple, but the same thing could be done to parse comma separated text (--supercell "1,1,1").

We'd just need to validate the length is either 3 or 9, and probably do type checking too.

[RastislavTuranyi]

Since there was no additional discussion, I have implemented the --supercell 1 2 3 and --supercell-matrix 1 2 3 4 5 6 7 8 9 version. Please let me know if you have any comments/suggestions/etc.!

[alinelena]

@RastislavTuranyi this will need to be rebased, can you ask @ajjackson to help you if you never done it before?

[ElliottKasoar]

Since there was no additional discussion, I have implemented the --supercell 1 2 3 and --supercell-matrix 1 2 3 4 5 6 7 8 9 version. Please let me know if you have any comments/suggestions/etc.!

Thanks for this!

Did you rule out the approach I suggested with building tuples similarly to how dictionaries are currently implemented?

To me, it feels nicer to have the same option, --supercell "(1, 2, 3)" or --supercell "(1, 2, 3, 4, 5, 6, 7, 8, 9)", and it could also allow us to take a single integer for the supercell through the CLI, which would be consistent with the Python interface.

Whether it's two similarly-named options, or one option that handles different lengths slightly differently, I think some ambiguity before reading the docs is unavoidable, so I don't think there's a perfect option.

I don't think the customs tuples have quite the same issues as we discussed with --supercell 1 2 3 vs --supercell 1 2 3 --supercell 4 5 6 --supercell 7 8 9, but it is admittedly less nice to manually parse the tuples than use the built-in tuples.

I'm ok with --supercell / --supercell-matrix, if other people who use phonopy etc more think it makes more sense, just want to check the options have been considered.

[alinelena]

I'm ok with --supercell / --supercell-matrix, if other people who use phonopy etc more think it makes more sense, just want to check the options have been considered.

technically there are three meaningful ways to specify a supercell, depending what you are after, single scalar n, for same repetition in all directions, most common choice, n1 n2 n3, maybe different numbers in all 3 directions, and the full matrix. given the other ones are shortcuts to the generalised version personally I will go with one --supercell, and based on the input, decide if we deal with generalised or one of the two shortcuts. I agree with @oerc0122 in using different names for things that do different things but in this case we do the same thing specify a supercell with different shortcuts.

[oerc0122]

I agree with @oerc0122 in using different names for things that do different things but in this case we do the same thing specify a supercell with different shortcuts.

I would argue that because the arguments are different types (3-tuple, 9-tuple, nothing in-between) It's pretty reasonable to specify them through different cmdline args so you have to be explicit and there's less confusion in scripts. For scalar -> supercell_matrix / scalar -> supercell I think that's a fairly obvious abbreviation with the same meaning, but the fact that 3-tuple passes to the diagonal only, it's a different situation.

@ElliottKasoar 's suggestion on the tuples is fine, but would a user expect to enter "(1, 2, 3, 4, 5, 6, 7, 8, 9)" or "((1, 2, 3), (4, 5, 6), (7, 8, 9))" would they be different? How about "(1, 2, 3)" or "(3,)" or "(3)"? What about if they put in "((1,), (2, 3), (4, 5, 6, 7, 8, 9))". It's a lot more effort to define in some senses.

[ElliottKasoar]

@ElliottKasoar 's suggestion on the tuples is fine, but would a user expect to enter "(1, 2, 3, 4, 5, 6, 7, 8, 9)" or "((1, 2, 3), (4, 5, 6), (7, 8, 9))" would they be different? How about "(1, 2, 3)" or "(3,)" or "(3)"? What about if they put in "((1,), (2, 3), (4, 5, 6, 7, 8, 9))". It's a lot more effort to define in some senses.

I'm open to alternatives, but I assumed we'd define the input as either an integer ("1") or 3- or 9-tuples ("(1, 2, 3)" or "(1, 2, 3, 4, 5, 6, 7, 8, 9)", which seems relatively simple to explain.

I think something like "(1)"is generally treated as an integer, (isinstance((1), int) is True), but if it didn't work I don't think too many people would complain.

We could also permit "(1,)", since for parsing we may want it in that form anyway, but again I wouldn't object if it were rejected.

[ajjackson]

I'm not a fan of parentheses in the string, unless we have line breaks as well it does very little to improve legibility.

Phonopy allows the user to provide 3 or 9 arguments to the same parameter, so the most "phonopy native" syntax is not really viable. String quoting with spaces in between numbers is an approximation to this. I would be fine with giving 9 values all the time but this is likely unpopular with other users...

I think having separate --supercell and --supercell-matrix arguments gives the least terrible type hints, given the limitations of Typer.

[ElliottKasoar]

I'm not a fan of parentheses in the string, unless we have line breaks as well it does very little to improve legibility.

Phonopy allows the user to provide 3 or 9 arguments to the same parameter, so the most "phonopy native" syntax is not really viable. String quoting with spaces in between numbers is an approximation to this. I would be fine with giving 9 values all the time but this is likely unpopular with other users...

I think having separate --supercell and --supercell-matrix arguments gives the least terrible type hints, given the limitations of Typer.

I wouldn't necessarily say it's about legibility, more just familiarity with how you would define it in the Python, and convenience for parsing it into an actual tuple. Removing the parenthesis and having space/comma separated options in a string would be a similar, reasonable solution, but would maybe feels a bit more arbitrary.

Again, I'm happy to go with the consensus, but when we already have a lot of options, and potentially more on the way with the refactor, splitting it into two feels a bit unnecessary. Also, while the type hint for each option's purpose may be slightly simpler, we'd probably also need to describe in at least one of them, maybe both, how they interact, which adds a bit more complication.

[ajjackson]

I wouldn't necessarily say it's about legibility, more just familiarity with how you would define it in the Python, and convenience for parsing it into an actual tuple.

Typically command-line arguments that would be mapped to a python list or tuple accept NARGS values directly; that's the familiar interface for such things. Typing Python code as a command-line argument feels a bit odd and uncomfortable; maybe because it alerts a primal fear that this string is about to get eval()ed!

[ElliottKasoar]

Typically command-line arguments that would be mapped to a python list or tuple accept NARGS values directly; that's the familiar interface for such things. Typing Python code as a command-line argument feels a bit odd and uncomfortable; maybe because it alerts a primal fear that this string is about to get eval()ed!

A legitimate fear! I agree it's very far from perfect, and unintuitive for CLI users, but I think we're stuck finding the least bad option.

My point about other alternatives being more arbitrary was that if it can't be done in a way that's familiar in terms of CLIs, mirroring the Python at least adds some level familiarity, and consistency with kwarg dict inputs, so it's probably easier to remember than a more arbitrary construct, even if it does look strange.

Splitting it into two options does allow nicer inputs, so the question is whether that's worth the added complication of two similar, competing options, no integer input option, and a less obvious mapping to phonopy.

I think we probably do need to settle on something though, at least between one or two command line options!

[ElliottKasoar]

In the interest of finishing this up, and after discussing it a little more with @alinelena, I think we should go for --supercell as the only input option, and then we should allow one, three, or nine integers, dealing with them as required.

I don't mind too much between "x y z" and "(x, y, z)", if it works. (Maybe the former is a little closer how we'd like the tuple input to be, so makes slightly more sense? I've only tried the latter though, so implementation may be tricker.)

Would that be ok, @RastislavTuranyi? If how to deal with the parsing etc. is unclear, I'm happy to help more.

[ajjackson]

I've only tried the latter though, so implementation may be tricker

Here are a couple of ideas

tuple(map(int, supercell.split())) # "1 2 3"

json.loads(supercell.replace("(", "[").replace(")", "]")) # "(1, 2, 3)"

Whitespace separation looks cleaner to me!

[oerc0122]

I've only tried the latter though, so implementation may be tricker

Here are a couple of ideas

tuple(map(int, supercell.split())) # "1 2 3"

json.loads(supercell.replace("(", "[").replace(")", "]")) # "(1, 2, 3)"

Whitespace separation looks cleaner to me!

For the latter

tuple(map(int, supercell.strip("()").split(","))

[ajjackson]

If the parser isn't going to look at the parens, why ask for them?

[oerc0122]

If the parser isn't going to look at the parens, why ask for them?

Agreed, I personally wouldn't expect to add parens to a CLI param.

[alinelena]

rebase

[RastislavTuranyi]

rebase

Done! I have also reverted to the string implementation, since that appeared to be the consensus?

[ElliottKasoar]

Looking good!

Could you please update the docs? In particular, the command line interface page of the user guide, but worth checking supercell doesn't appear anywhere else.

My only other thought is if we can make the docstrings slightly more concise, but if in doubt I'm happy to err on the side of clarity.

[RastislavTuranyi]

Could you please update the docs? In particular, the command line interface page of the user guide, but worth checking supercell doesn't appear anywhere else.

I have updated the code examples to use quotes for the integers, but would you like me to add a paragraph explaining the various possible inputs for --supercell as well? Or maybe just reuse the existing code examples by passing in the supercell using a different format in each?

I haven't found any mentions of phonons or supercell in the rest of the .rst documents; they either don't mention it or are auto-generated. Maybe the jupyter notebook examples should be updated?

[ElliottKasoar]

I have updated the code examples to use quotes for the integers, but would you like me to add a paragraph explaining the various possible inputs for --supercell as well? Or maybe just reuse the existing code examples by passing in the supercell using a different format in each?

I haven't found any mentions of phonons or supercell in the rest of the .rst documents; they either don't mention it or are auto-generated. Maybe the jupyter notebook examples should be updated?

Thanks, @RastislavTuranyi!

It might be nice to add a quick note in the CLI docs about how supercell can be defined, if that's ok, but it's not essential.

I need to do another pass on the tutorials to make sure they're still consistent with the newest changes, so I'll handle those separately, thanks!

I can't comment on the code, but the supercell : MaybeList[int] description for the Phonons class parameters needs updating to match the __init__ definition.

Would be good if anyone else who uses Phonopy can confirm they're happy, but other than the minor suggestions (/typo corrections) I think this is good to go from my perspective

[RastislavTuranyi]

Everything should be updated now, though please do let me know if the CLI documentation is too verbose or not detailed enough.

[ElliottKasoar]

Everything should be updated now, though please do let me know if the CLI documentation is too verbose or not detailed enough.

I think the section you added is a good amount, thanks! Generally I lean towards more concise wording for something like the output from --help, so it doesn't fill up the terminal, but slightly more verbose for the full .rst docs, since space is less of a problem, but either way clarity and unambiguity should come first.

Thank you everyone for the work that's gone into this!