-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance Define Climatic Dialogue Autofill #9275
base: master
Are you sure you want to change the base?
Conversation
@N-thony you have missed the main point in item a) in #9272 . I have now put a bit there in bold! Now you have fixed to recognise max and min, you can leave that. But the point is that occasionally the dialog will NOT recognise certain elements, and you then have to fill yourself what they are. That's fine. What is going wrong is that when you return to that fialog in another session, having opened that same file, then it should remember the entries you typed manually, because they are still the climatic elements - and recognised in the metadata. But it doesn't. You could try, with dodoma is you rename tmin to say nnn and tmax as xxx. Then use the dialog. Then save the file and close R-Instat. Then return and open these data again. In the data from Bangladesh it now recognises max and min, but not wind - which is the wind speed. I am happy to fill in manually, but it then needs to fill in for me, when I return to the dialog in a later session. I hope that's clear now? |
@rdstern, I get it now and this becomes more interesting in how we would dynamically add new element in the list. Let me see if that is possible with the current implementation where we predefined elements, otherwise we will have to change it. |
@rdstern have a look now, this will use the metadata property to enhance the autofill. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataBook$set("public", "get_column_climatic_type", function(data_name, col_name, attr_name){ | ||
self$get_data_objects(data_name)$get_column_climatic_type(col_name = col_name, attr_name =attr_name) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@N-thony, this looks good on the VB side. I’ll proceed with merging it after @lilyclements provides feedback.
@lilyclements, @N-thony has added a new R function, get_column_climatic_type
, in the data book. Do you think this was necessary, or could an existing function have been reused? Since you recently worked on the data book and climate-related functions, your input would be valuable here.
When I was making significant changes to the R data book previously, I noticed a number of R functions performing similar tasks where a single function with a parameter could suffice (what I refer to as "code bloats"). I worked to remove redundant functions during my updates to the output objects functions. Your thoughts on this would be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Patowhiz - Interesting, I've not heard the term "code bloats". Can you show an example of the sort of thing you have done? (e.g., a PR?)
@N-thony how does get_column_climatic_type
differ from get_column_data_types
?
Can you give a simple example of get_column_climatic_type
?
Could it not be instead that attr_name
is default NULL
and an option in get_column_data_types
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilyclements get_column_data_types
retrieves the data type/class for a given column and get_column_climatic_type
retrieves a specific climatic type attribute for a column. So, in this case, we would like to autofill the receivers with corresponding variables based on their climatic type attributes. I hope this okay.
Fixes #9272
@rdstern have a look? This fixes items a) b)