sirocco-rt / sirocco

This is the repository for Sirocco, the radiative transfer code used to model winds in AGN and other systems
GNU General Public License v3.0
28 stars 24 forks source link

rdchoice - Choosing 1 of n options alphanumerically #362

Closed kslong closed 5 years ago

kslong commented 6 years ago

We are systematizing keywords for IO and rearranging their order, but so far we have not dealt with the problem that some modes are deprecated, that for the "choose 1 of n options" type keywords, we have skipped numbers, etc. A good example of this is system type, which needs expansion so that we can add separate entries for x-ray binaries, Ostars, etc, but we cannot put them in a logical order using numbers (or at least we cannot without a good deal of confusion.

If we were going to improve this what would be the requirements:

@jhmatthews suggested some time ago that one part of the solution was to use alphanumeric entries for these types of inputs. His reasoning I believe were that they were clearer, but this also has the advantage that one can rearrange, add, and subtract items like this more easily than a number-based approach. I think he is probably right.

An alphanumeric input works in terms of backwards compatibility, because is is possible to read a number in as a string, and thus one could for example have both a number and a true string read in and use either to set the internal variable we use to describe an option. To implement this, we would just replace some of the rdint's with rdstr.

To deal with deprecated options (all of the numbers would become deprecated), we would probably need some kind of input record structure, e.g

struct record { char input_name[20]; int internal_number int status; } where the input_name is what the user enters, internal_number is the associated #define, and status indicates whether this is currently to be seen by the user, is deprecated, or in principle is no longer used.

I suspect one would have to create an array of these structures, or another structure, for a single input variable such as system_type, or line_transfer_mode. One would need to have some way of knowing how many possibilities there were, either by including this explicitly, or by using the approach we are using in synonyms, where the last element of an array is a NULL.

The reason I am thinking of this right now is for system_type where if we had a system type for an xray binary (xrb) and for an agn we could set variables such as the central object mass to be typical for that type of system. We could also separate CVs from Ostars, etc.

Comments

kslong commented 6 years ago

As discussed in Belfast, we do propose on some timescale to go ahead with this update, which would allow one to move toward a system where strings rather than intergers are entered in the .pf file for variables that really represent several discrete choices. And example of this would be line_transfer mode.

. The idea for the implementation is something like the following:

struct input_par { int npars; // the number of possible choices char variables[NPAR_MAX] // A series of strings that can be mapped onto the internal mode char modes[NPAR_MAX] // A series of internal integers, usually but not necessarily set by #define statements. Many to 1 matches are allowed, especially during a transition period } test;

Comments, in principle the line written to the screen can incorporate the non-integer values of variables, and the new call could read something like

answer=xstring(question,&test);

or xstring(question,&test,default);

(Note: as an aside we also agreed at Belfast that we would begin to allow changes in the inputs that were not completely backward compatible, as there are cases this is starting to restrict what we do, examples of this would be where we wanted to split the functionality of input variables to something more sensible. ksl had some python routines to make it easier to update old to new formats. If he remembers correctly what they did was to start with a new input file, and then to interpolate values from an old file into the new one, when the names were the same. ksl should look for this.)

kslong commented 6 years ago

@jhmatthews @Higginbottom I have implemented a new capability into rdpar, called rdchoice which is intended to allow for string inputs when we want to make a choice between various options.

The following indicates how to change from the old way of getting a choice to a new way

Old

geo.system_type=0
rdint ("System_type(0=star,1=binary,2=agn,3=previous)", &geo.system_type);

New

strcpy(answer,"star");
geo.system_type=rdchoice("System_type(0=star,1=binary,2=agn,3=previous)","1,2,3",answer);

With the new approach, it is important to realize that one can not just set answer to a string,

char  answer="star"

but rather one must allocate answer and then copy into it.

char answer[LINELENGTH];
strcpy(answer,"star");

If one does not do this correctly, one will get a segfault, when one tries to modify answer.

At present, I have only updated one of the input parameters to use rdchoice, namely geo.system_type. Right now this would be easy to back out, so if you do not think this is a good route to go, this would be the time to raise objections. Otherwise, I will gradually move down this path.

Note that at present rdchoice is backward compatible in the sense that it is still able to interpret integer inputs ... but doing this is a bit dangerous, because it implies one was not give a string choice that begins with an integer. Something like "1d" would fail, and return 1.

kslong commented 6 years ago

This should be working now, but one enhancement that would be worthwhile would be to get the routine to write the full answer to the output .pf file. Right now, it writes what the user enters, which is minimum matched to the possibilities. This is not necessarily straightforward, because rdchoice right now uses rdstring to handle the underlying interaction with the user.

kslong commented 6 years ago

I have updated a large number but not all of the rdint's that should be switched to rdchoice. It is still possible to use all of the old input files, but warning is placed in the .out.pf file. It should be possible to write a simple python script to read and fix the .out.pf files so that it replaces the integer value with the string that is supposed to be there.

More testing that this is all working properly, in the sense that all of the replacements of rdints with rdchoice are correct.

kslong commented 6 years ago

I have checked that most if not all of the input variables read in with rdint should not be using rdchoice. The exceptions are:

kslong commented 6 years ago

From my perspective rdchoice is now fully functional. The only reason for keeping this issue open is that not all of the rdints that should be have been converted to rdchoice. Others, besides ksl, should be able to make the changes needed to do this.

If anyone discovers a basic problem with the functionality of rdchoice, they are encouraged to provide the .pf file that crated the problem, and ksl will try to address it.

kslong commented 5 years ago

There a few variables that still need to be converted to rdchoice

kslong commented 5 years ago

I uncovered a small problem associated with rdchoice. Currently if you start python in interactive mode, some of the rdchoice lines are being printed out twice to the .out.pf file.

It turned out this was happening only when the user opted to accept the default option with carriage return. The fix was straightforward once identified.

kslong commented 5 years ago

This activity is essentially complete. There may be a few other variables that we should change to rdchoice, but nearly everything is done, so I will close this issue.