search

United4Surveillance / signal-detection-tool

A tool for detection of signals in infectious disease surveillance data.
Other
8 stars 0 forks source link

Feature/restructure and rename tables #291

Closed tinneuro closed 3 months ago

tinneuro commented 4 months ago

Preparations done due to sharing most important functions for integration into local systems.

Work on the results_table.R script:

tinneuro commented 4 months ago

Hi @tomasmartbert ,

can you manage to do the review today? Thanks!

tomasmartbert commented 4 months ago

Nice work! Here a few preliminary comments - I'll get back with some suggestions later.

Preparations done due to sharing most important functions for integration into local systems.

  • Aggregation of linelist is now a public function, not ultimately necessary as aggregation is done inside the get_signals() but I think good to have.

Should we consider exposing only the minimal set of functions, hence not export aggregation function?

Work on the results_table.R script:

  • seperating the formating and preprocessing of tables such that users can also only retrieve the preprocessed tables using our functions
  • renaming the functions to improve clarity (hopefully)

I feel the names are little too long and could be made more precise - but otherwise a nice refactoring.

tinneuro commented 3 months ago

I have to think a bit more on this and do some testing. Here are some preliminary thoughts, though. Perhaps it would make sense to control the output type through options to a single make_signals_table() instead of a splitup of the signal table function?

That is a good point. I am also thinking about it and can maybe implement an example alternative.

I feel the names are little too long and could be made more precise - but otherwise a nice refactoring. Yeah I agree. You have any suggestions? But maybe first I try the above mentioned improvement.

tinneuro commented 3 months ago

Should we consider exposing only the minimal set of functions, hence not export aggregation function?

Yes let's start with your suggestion. We can also discuss this in the next tool dev meeting and we can see whether some countries who want to integrate it into their systems need more public functions.

I have to think a bit more on this and do some testing. Here are some preliminary thoughts, though. Perhaps it would make sense to control the output type through options to a single make_signals_table() instead of a splitup of the signal table function?

I also thought a bit and my first intuition is that I would tend to keep them as two seperate functions as I think it improves testability of each individual one and maybe also readability of the code. For the user to just have one function might be nice I agree. But if you have good arguments to put it in one happy to do that. I will work on the naming now.

tinneuro commented 3 months ago

@tomasmartbert Are you back today to review? I want to get this merged today for the new version.

tomasmartbert commented 3 months ago

@tomasmartbert Are you back today to review? I want to get this merged today for the new version.

Hi @tinneuro I don't have much time the rest of this week - I'll see if I can make a quick look today.

tinneuro commented 3 months ago

Thanks, If not no problem maybe @alchalu or @aj-thl can review otherwise

tomasmartbert commented 3 months ago

Should we consider exposing only the minimal set of functions, hence not export aggregation function?

Yes let's start with your suggestion. We can also discuss this in the next tool dev meeting and we can see whether some countries who want to integrate it into their systems need more public functions.

Yes, I think it would be a good thing to discuss at the meeting - it is maybe of more conceptual character.

I have to think a bit more on this and do some testing. Here are some preliminary thoughts, though. Perhaps it would make sense to control the output type through options to a single make_signals_table() instead of a splitup of the signal table function?

I also thought a bit and my first intuition is that I would tend to keep them as two seperate functions as I think it improves testability of each individual one and maybe also readability of the code. For the user to just have one function might be nice I agree. But if you have good arguments to put it in one happy to do that. I will work on the naming now.

That's a good point regarding testability, I think. I'm not so familiar with these functions so I'll yet have to dig into the code to give some more intelligible feedback. From the user's point of view a simple wrapper function to be exposed might work to not be overwhelmed by the sheer number of functions. E.g. something in line with make_signals_table(data, options, format = c("data.frame", "gt", "DataTable"))

alchalu commented 3 months ago

I've noticed that I'm struggling a bit myslef to use the various functions outside of the app, especially after not looking at the project for a while. I think we still have room to improve with userfriendliness of everything that's exported outside the app. I agree this is more of a conceptual issue, and I'm not sure if this PR is the right place to discuss it.

But maybe for the next meeting it would be a good step to discuss and define all the functions that we want to make available for the user.

tinneuro commented 3 months ago

Thank you @alchalu and @tomasmartbert for your valuable review and feedback! Yes I agree with you @alchalu I especially had this now when I was working on the signals tab, the report and these tables that I started a bit to clean up and I am very up for discussing which functions we want to make available

tinneuro commented 3 months ago

@tomasmartbert thanks for your suggestion make_signals_table(data, options, format = c("data.frame", "gt", "DataTable")) this was good. I changed my build_signals_table() now so it can take an argument like format and I think and hope it is nice and intuitive to use now