irreducible-representations / irrep

GNU General Public License v3.0
62 stars 31 forks source link

Add ability to load settings from a configuration file #26

Closed mkhorton closed 3 years ago

mkhorton commented 3 years ago

Tested on the Bi-scalar example, where you can now run irrep -config config.yml and the contents of config.yml are as follows:

Ecut: 50
code: "abinit"
fWFK: "O_DS2_WFK"
refUC: "0,-1,1,1,0,-1,-1,-1,-1"
kpoints: "1"
IBstart: 11
IBend: 15
kpnames: "GM" 

The filename can be anything, but should end in .yml if providing the input in YAML format or end in .json if providing the input in JSON format.

If we set a standard input file name, I could also set this to look in the current directory for that file name, so that no option need be supplied and could the command could just be run using irrep.

stepan-tsirkin commented 3 years ago

Thank you Matthew

f we set a standard input file name, I could also set this to look in the current directory for that file name, so that no option need be supplied and could the command could just be run using irrep

Yes, could you set such a efault file name. config.yml is too general, the directory may have other configs related to ab initio codes or whatever calculation. maybe irrep-config or similar.

Can the code also read the input file and command-line arguments at the same time? Say, so that command-line parameters override those from the file, if they are found in both?

mkhorton commented 3 years ago

Yes, could you set such a efault file name. config.yml is too general, the directory may have other configs related to ab initio codes or whatever calculation. maybe irrep-config or similar.

Ok, I'll think of a name. Perhaps irrep.in?

Can the code also read the input file and command-line arguments at the same time? Say, so that command-line parameters override those from the file, if they are found in both?

Yes, both can be input at the same time, but currently the settings in the config file take precedence.

stepan-tsirkin commented 3 years ago

Ok, I'll think of a name. Perhaps irrep.in? fine for me Yes, both can be input at the same time, but currently the settings in the config file take precedence. I think it is logical to make it the other way round. Say, the file stores the 'defaults' (for the type of calculations in question, of the favorite settings of the user) and the command-line options are to deviate from those when needed.

Just out of curiosity : what will happen if one writes in the config file config: "other_config.yml" ?

mkhorton commented 3 years ago

Just out of curiosity : what will happen if one writes in the config file config: "other_config.yml" ?

That's a great question! It is ignored. (To be more precise, the config variable is indeed overwritten with other_config.yml, but the variable is no longer used at this point since the configuration file has already been loaded.)

stepan-tsirkin commented 3 years ago

Ok, that makes sense.

stepan-tsirkin commented 3 years ago

Can the code also read the input file and command-line arguments at the same time? Say, so that command-line parameters override those from the file, if they are found in both?

Yes, both can be input at the same time, but currently the settings in the config file take precedence.

Hi, @mkhorton ,

can you make it the other way round in the nearest days? So that the command line takes precedence over the file. We are wrapping up the code to resubmit next week.

mkhorton commented 3 years ago

The code will be simpler if we allow the config file to take precedence, since the command-line options have pre-defined defaults, so it is not obvious whether the option supplied is the default value or a user-supplied value. Since there is no config file by default, it is always the case that the config file options are user-supplied values, and there is no ambiguity. Therefore, I think it would be better to keep it the way originally proposed?

stepan-tsirkin commented 3 years ago

Ok, I see, let's leave it like this.

After merging the recent developments from master, the checks fail. @mkhorton , can you resolve this? @MIraola do you see what the problem is?

mkhorton commented 3 years ago

It seems to be an issue involving shiftUC and refUC: https://github.com/stepan-tsirkin/irrep/runs/3022647922#step:5:177

stepan-tsirkin commented 3 years ago

I made a fix. @MIraola , @mkhorton what do you think?

mkhorton commented 3 years ago

Looks fine to me, I pushed a small formatting change.

Really, this determine_basis_transf function specifies that it accepts arrays, we shouldn't be doing type conversion within the function itself (or the docstring should be more clear). For now, I think this works though.

In that function, the flag variable is also defined but not used.

MIraola commented 3 years ago

Hi, it think that your the corrections to determine_basis_transf are good. But I think that the definitions in lines 900 and 901 can go inside the if statements in 896 and 898, respectively.

stepan-tsirkin commented 3 years ago

Really, this determine_basis_transf function specifies that it accepts arrays, we shouldn't be doing type conversion within the function itself (or the docstring should be more clear). For now, I think this works though.

Yopu are right, the transformation is not necessary. I commented out those lines, and tests pass