SamAmco / track-and-graph

An android app for tracking personal data and creating custom graphs
GNU General Public License v3.0
438 stars 40 forks source link

Refactor and extend aggregation and more #133

Closed hhpmmd closed 2 years ago

hhpmmd commented 3 years ago

Hi, so this PR does a bunch of things:

  1. Some tests failed on my PC It turned out that some tests rely on the first day of the week being Monday. On some locales (mine) this is set to Sunday. I introduced the AggregationWindowPreferences to allow to externally set this.

  2. AggregatedDataPoint I wanted to have the AggregatedDataPoint class for the work on the data transformation branch. I implemented it using an interface since you cant inherit from the original DataPoint class (it's a data class)

  3. Aggregation Refactor Since i want to aggregate using all kinds of functions with the datatransformation language, i thought it would be a good time to refactor the function for reusability

  4. Start of Day I saw it recently in #130 and had the idea before myself and since I was working on the code I thought why not implement this as well. It should be fully implemented, but there is currently no way to alter this start-of-day value. Maybe it is time to do global settings/preferences (which is UI stuff :D). Allowing to set the first day of the week should also be added there I think (who really starts the week on Sundays????)

I refactored the findBeginningOfDuration function because I thought i had to change it more, but didn't. I think the code is clearer now and it should do the same. I can understand if you want to use the old implementation to be 100% sure the behavior doesn't change.

hhpmmd commented 3 years ago

In summary this PR doesn't change, add or remove any functionality for users, it just allows for various extensions

SamAmco commented 2 years ago

Probably un-related to this PR but i still have one failing test and i thought i should let you know about it since i believe it relates to your earlier work:

No solution found! No intervals passed the range_used check. ymin: 74.7, ymax: 110.2 java.lang.Exception: No solution found! No intervals passed the range_used check. ymin: 74.7, ymax: 110.2 at com.samco.trackandgraph.statistics.StatisticsKt.getYParametersInternal(Statistics.kt:867) at com.samco.trackandgraph.statistics.StatisticsKt.getYParameters(Statistics.kt:783) at com.samco.trackandgraph.statistics.Statistics_getYParameters_KtTest.printExamplesNumerical(Statistics_getYParameters_KtTest.kt:99)

SamAmco commented 2 years ago

Just ran the app on this branch and when i clicked on a graph to open it in full screen i got the following exception:

java.lang.ClassCastException: com.samco.trackandgraph.viewgraphstat.ViewGraphStatViewModel$initFromGraphStatId$viewData$1 cannot be cast to com.samco.trackandgraph.database.dto.LineGraphWithFeatures

Can you please check full screen graphs are working for all graph types. Also please try to stress test the graph setup screen for all graph types.

SamAmco commented 2 years ago

So I presume that this crash is due to a failed cast between DataPoint and DataPointInterface. To that point though there are still a lot of references to DataPoint in the code. Most likely almost all of them should reference DataPointInterface and not DataPoint. You can do find references in Android Studio.

SamAmco commented 2 years ago

Ok other than this i'm happy to merge this PR if you think it will help you. Bear in mind that you have targeted this at the master branch. Again I don't mind too much because as you say it may be helpful for other features but I guess we will need to merge it to the data_transformation branch as well.

The only thing I would say is if you can add any comments to the functions in Statistics.kt that you have implemented/refactored using the / comments / syntax, that would be much appreciated. Try to explain the expected behaviour of the function because when you come back in a few months because something is not quite right it can be hard to remember whether the behaviour you have is expected or a bug/edge case you weren't aware of.

After this PR i would really like to get rid of Statistics.kt all together. It was only ever meant to be a temporary solution but i never got around to refactoring it. In general i think it's pretty poor form to keep a file full of global functions with little relation to each other. I will probably create a library of classes like: DataAggregationCalculator, DataAveragingCalculator etc and move all the code to those. Then you can create an instance of those classes to get the results you want.

hhpmmd commented 2 years ago

I fixed the test and went ahead and split Statistics.kt into three files and added some comments to my code.

hhpmmd commented 2 years ago

I also fixed the crash by changing some types from DataPoint to DataPointInterface as you anticipated. I really should have catched that one by myself though, I underestimated the ripple effect of introducing that interface.

Unfortunately we can't just change all references to the interface as some deal with creating/deleting/modifying the datapoints (not possible for the aggregated ones). I went through the cases and from what I can tell, the usages now should be along those lines.

hhpmmd commented 2 years ago

And regarding targeting the master: I chose this target because of the quality of live updates and the start of day functionality. After that I'll merge it into the data_transformation branch which should be straightforward since I haven't modified Statistics.kt on that branch

SamAmco commented 2 years ago

I also fixed the crash by changing some types from DataPoint to DataPointInterface as you anticipated. I really should have catched that one by myself though, I underestimated the ripple effect of introducing that interface.

Unfortunately we can't just change all references to the interface as some deal with creating/deleting/modifying the datapoints (not possible for the aggregated ones). I went through the cases and from what I can tell, the usages now should be along those lines.

Yeah my bad. This is why you shouldn't pass entity classes around the code. It was just laziness really.

SamAmco commented 2 years ago

Merged :)