lardemua / atom

Calibration tools for multi-sensor, multi-modal robotic systems
GNU General Public License v3.0
245 stars 28 forks source link

Executing softbot experiments #948

Open brunofavs opened 1 month ago

brunofavs commented 1 month ago

This issue is a followup of #926.

I have everything ready to run the experiments tonight. I'm not sure my pc will handle it well considering that calibrating the odom tf adds a lot more optimization parameters, will see.

I was missing -atsf

This was something critical that was actually missing, my baseline. I have updated the template to add this feature and right now its like this :

     ...
     rosrun atom_calibration calibrate -json {{ dataset_path }}/dataset_corrected.json \
          -v  -ss {{ run }} \
          -nig {{ e.nig_value }} {{ e.nig_value }} \
          -csf 'lambda x: int(x) in {{ fold[0] }}' \
          -ntfv {{ e.ntfv_value }} {{ e.ntfv_value }} \
          -ntfl "world:base_footprint" \
          -sce \
          {% if not e.calibrate_odom -%}
            -atsf 'lambda name : name in []' \
          {%- endif %}
          && \
          ...

Basically in some experiments I have a boolean excluding all additional tf's.

Im not sure how to evenly distribute all the variance of all variables I want to study. Rn I'm varying nig , and ntfv between 0 and 3 m/rad and atsf between true/false. I feel I have way too many experiences for my pc to handle in feasible time.

This is my data.yml at the moment :

#
#           █████╗ ████████╗ ██████╗ ███╗   ███╗
#          ██╔══██╗╚══██╔══╝██╔═══██╗████╗ ████║
#          ███████║   ██║   ██║   ██║██╔████╔██║
#          ██╔══██║   ██║   ██║   ██║██║╚██╔╝██║
#   __     ██║  ██║   ██║   ╚██████╔╝██║ ╚═╝ ██║    _
#  / _|    ╚═╝  ╚═╝   ╚═╝    ╚═════╝ ╚═╝     ╚═╝   | |
#  | |_ _ __ __ _ _ __ ___   _____      _____  _ __| | __
#  |  _| '__/ _` | '_ ` _ \ / _ \ \ /\ / / _ \| '__| |/ /
#  | | | | | (_| | | | | | |  __/\ v  v / (_) | |  |   <
#  |_| |_|  \__,_|_| |_| |_|\___| \_/\_/ \___/|_|  |_|\_\
#  https://github.com/lardemua/atom

# this yml file contains variables to be used in conjunction with batch.yml

# Auxiliary variables, to be used to render other fields in the template.yml.j2 file
package_path: "package://softbot_calibration"
dataset_path: '$ATOM_DATASETS/softbot/long_train_dataset1'
collections_to_remove: [27,36,37]

# Runs are repetitions of the experiments for gathering statistically significant results
runs: [1,2,3]

cross_validation:
  type: "stratified-k-fold" 
  n_splits: 5 # Number of folds
  train_size:  # Percentage of the dataset used for training, only used in StratifiedShuffleSplit

# Experiments are executions with a set of input parameters
experiments:
# Varying noise from 0 to 0.3m/rad

# --------------------
# Calibrating odom
# --------------------

  # Perfect Simulation
  - {name: perfect_sim, nig_value: 0.0, ntfv_value: 0.0, calibrate_odom : true}

  # Varying nig only
  - {name: nig_0.1,  nig_value: 0.1,  ntfv_value: 0.0,  calibrate_odom : true}
  - {name: nig_0.15, nig_value: 0.15, ntfv_value: 0.0,  calibrate_odom : true}
  - {name: nig_0.2,  nig_value: 0.2,  ntfv_value: 0.0,  calibrate_odom : true}
  - {name: nig_0.25, nig_value: 0.25, ntfv_value: 0.0,  calibrate_odom : true}
  - {name: nig_0.3,  nig_value: 0.3,  ntfv_value: 0.0,  calibrate_odom : true}

    #Varying ntfv with fixed nig = 0.1
  - {name: nig_0.1-ntfv_0.1 ,  nig_value: 0.1,  ntfv_value: 0.1,   calibrate_odom : true}
  - {name: nig_0.1-ntfv_0.15,  nig_value: 0.1,  ntfv_value: 0.15,  calibrate_odom : true}
  - {name: nig_0.1-ntfv_0.2 ,  nig_value: 0.1,  ntfv_value: 0.2,   calibrate_odom : true}
  - {name: nig_0.1-ntfv_0.25,  nig_value: 0.1,  ntfv_value: 0.25,  calibrate_odom : true}
  - {name: nig_0.1-ntfv_0.3 ,  nig_value: 0.1,  ntfv_value: 0.3,   calibrate_odom : true}

  #   #Varying ntfv with fixed nig = 0.15 
  - {name: nig_0.15-ntfv_0.1,  nig_value: 0.15, ntfv_value: 0.1,  calibrate_odom : true}
  - {name: nig_0.15-ntfv_0.15, nig_value: 0.15, ntfv_value: 0.15, calibrate_odom : true}
  - {name: nig_0.15-ntfv_0.2,  nig_value: 0.15, ntfv_value: 0.2,  calibrate_odom : true}
  - {name: nig_0.15-ntfv_0.25, nig_value: 0.15, ntfv_value: 0.25, calibrate_odom : true}
  - {name: nig_0.15-ntfv_0.3,  nig_value: 0.15, ntfv_value: 0.3,  calibrate_odom : true}

  #   #Varying ntfv with fixed nig = 0.2
  - {name: nig_0.2-ntfv_0.1 , nig_value: 0.2, ntfv_value: 0.1,  calibrate_odom : true}
  - {name: nig_0.2-ntfv_0.15, nig_value: 0.2, ntfv_value: 0.15, calibrate_odom : true}
  - {name: nig_0.2-ntfv_0.2 , nig_value: 0.2, ntfv_value: 0.2,  calibrate_odom : true}
  - {name: nig_0.2-ntfv_0.25, nig_value: 0.2, ntfv_value: 0.25, calibrate_odom : true}
  - {name: nig_0.2-ntfv_0.3 , nig_value: 0.2, ntfv_value: 0.3,  calibrate_odom : true}

  #   #Varying ntfv with fixed nig = 0.25 
  - {name: nig_0.25-ntfv_0.1,  nig_value: 0.25, ntfv_value: 0.1,  calibrate_odom : true}
  - {name: nig_0.25-ntfv_0.15, nig_value: 0.25, ntfv_value: 0.15, calibrate_odom : true}
  - {name: nig_0.25-ntfv_0.2,  nig_value: 0.25, ntfv_value: 0.2,  calibrate_odom : true}
  - {name: nig_0.25-ntfv_0.25, nig_value: 0.25, ntfv_value: 0.25, calibrate_odom : true}
  - {name: nig_0.25-ntfv_0.3,  nig_value: 0.25, ntfv_value: 0.3,  calibrate_odom : true}

  #   #Varying ntfv with fixed nig = 0.3
  - {name: nig_0.3-ntfv_0.1,  nig_value: 0.3, ntfv_value: 0.1,  calibrate_odom : true}
  - {name: nig_0.3-ntfv_0.15, nig_value: 0.3, ntfv_value: 0.15, calibrate_odom : true}
  - {name: nig_0.3-ntfv_0.2,  nig_value: 0.3, ntfv_value: 0.2,  calibrate_odom : true}
  - {name: nig_0.3-ntfv_0.25, nig_value: 0.3, ntfv_value: 0.25, calibrate_odom : true}
  - {name: nig_0.3-ntfv_0.3,  nig_value: 0.3, ntfv_value: 0.3,  calibrate_odom : true}

# --------------------
# Not calibrating odom
# --------------------
#
# Not sure which to repeat without calibrating odom
  - {name: nig_0.2_no_odom_calib,           nig_value: 0.2,  ntfv_value: 0.0,  calibrate_odom : false}
  - {name: nig_0.2-ntfv_0.2_no_odom_calib,  nig_value: 0.2,  ntfv_value: 0.2,  calibrate_odom : false}
brunofavs commented 1 month ago

Meanwhile I found another hiccup.

I have 3 truly horrendous collections in my dataset. I don't wan't them anywhere. They weren't being excluded before.

I added this to the batch execution script :

https://github.com/lardemua/atom/blob/6807048fee8f227c8500262054d7cd50a09ec362/atom_batch_execution/scripts/batch_execution#L79-L87

I changed the script to use the dataset_corrected.json as the class divider for the cross validator relies on the label detections and these in the non corrected dataset when LiDARs are present are really poor and unrepresentative.

I excluded for the same reason the collections here. I don't want them to interfere in the class divider if they are meaningless.

Moreover, I added this in the template to ensure these collections aren't used anywhere, not even in evaluation :

https://github.com/lardemua/atom/blob/6807048fee8f227c8500262054d7cd50a09ec362/atom_batch_execution/experiments/softbot_example/template.yml.j2#L33

The same was done to every script running.

PS I do realize in the last permalink there should've been a 'not' in the second clause, it was a oopsie, its solved already :)

I would like some feedback @miguelriemoliveira :)

By tomorrow I should have some results, will leave running tonight.

brunofavs commented 1 month ago

Small fix e4168b46430c

Forgot collection keys are '010' and not just a integer.

Added a function specific to this in dataset.io. Very similar to filterCollectionsFromDataset

brunofavs commented 1 month ago

Another hiccup, this one I think I require @manuelgitgomes' guidance.

Initial problem

When deleting a collection from a dataset, it still appears in the folds.

(I triple checked that they were actually removed)

Explanation

Whenever we remove a collection, it no longer is in the dataset. This means the index 30 ( the 30th collection, no longer has necessarily the collection key '030', if a collection before it was removed.

When we split in folds, here : https://github.com/lardemua/atom/blob/e4168b46430ceb7eb17a770db392b59c15583d09/atom_batch_execution/scripts/batch_execution#L125

The splits return only the indices. This is confirmed by looking at the rendered yaml and on the wiki

https://github.com/lardemua/atom/blob/e4168b46430ceb7eb17a770db392b59c15583d09/atom_batch_execution/experiments/softbot_example/auto_rendered.yaml#L32

The indexes 26 36 37 don't exist in the dataset. They show here regardless because the folds were based on indexes.

This might problematic because there is a missmatch of collections.

If I were to remove the first half(20) of all(40) collections, the split would only give me indexes until 20 even though the "good collections" are from 21 to 40 and crash because the -csf would give me a empty list of collections.

brunofavs commented 1 month ago

It should'nt be a super hard fix though, although it would be difficult to find in the future.

brunofavs commented 1 month ago

Thankfully since Python 3.7 dictionaries became ordered regardless of being hash tables, otherwise this would've been a much bigger problem

https://github.com/lardemua/atom/blob/6a117f70e61df834e1168b328d47713bfa71d2a7/atom_batch_execution/scripts/batch_execution#L135-L147

It's now fixed, the collections shown in the rendered yaml now are actual collection keys and not indexes.

I also created this small function to use in the future to generate collection keys :

https://github.com/lardemua/atom/blob/6a117f70e61df834e1168b328d47713bfa71d2a7/atom_core/src/atom_core/naming.py#L15-L16

IT just converts int 1 to '001' for instance.

brunofavs commented 1 month ago

https://github.com/lardemua/atom/issues/948#issuecomment-2097287626

Moreover, I added this in the template to ensure these collections aren't used anywhere, not even in evaluation :

https://github.com/lardemua/atom/blob/6807048fee8f227c8500262054d7cd50a09ec362/atom_batch_execution/experiments/softbot_example/template.yml.j2#L33

I removed this second clause because now the folds don't have these bad collections so it doesn't do anything there.

miguelriemoliveira commented 1 month ago

Hi @brunofavs ,

I told you there should be still many problems to solve. But it looks you're on the right track.

I think your questions are more on @manuelgitgomes 's side. If you need my help let me know.

manuelgitgomes commented 1 month ago

Hello @brunofavs. I agree with @miguelriemoliveira that you are on the right track and had the ability to solve the bumps on the road that appeared. Congratulations!

However, I have some disagreements with some of your suggestions/changes:

I added this to the batch execution script :

I think this was not a good idea. You are adding a new field to a complex system that will make all the old templates outdated. Please keep that in mind when changing things around ATOM in general. We already have a script that removes collections from a dataset: https://github.com/lardemua/atom/blob/noetic-devel/atom_calibration/scripts/utilities/remove_collections_from_atom_dataset I suggest that you remove that part of the batch execution and erase the problematic collection of your dataset using the previous script.

I changed the script to use the dataset_corrected.json as the class divider for the cross validator relies on the label detections and these in the non corrected dataset when LiDARs are present are really poor and unrepresentative.

dataset_corrected.json is not a mandatory part of the ATOM pipeline, so it should not be necessary to use in the batch execution. The cross validator relies on the detected parameter of the labels. Even if you change the labeling, I believe this parameter is not changed most of the times. I agree that in some situations it might be necessary. However, I suggest you add a flag or some other way to use dataset_corrected.json optionally, while using dataset.json as a default.

Added a function specific to this in dataset.io. Very similar to filterCollectionsFromDataset

I think having two of these functions is redundant. To have consistency, I suggest using only the filterCollectionsFromDataset. It also does not make the assumption that collection keys are "converted" integers, as the one you created does. This is more of a code organization issue, so I will ask for the option of @miguelriemoliveira about this specific problem.

Another hiccup, this one I think I require @manuelgitgomes' guidance.

Sorry, this one is totally my fault. Thank you for fixing it, but I saw a simpler way to do it: https://github.com/lardemua/atom/blob/913c5bf4ed323c936836d410d06370fecace90d4/atom_batch_execution/scripts/batch_execution#L134-L138 I am also changing the example templates to remove collection conversion to integer: https://github.com/lardemua/atom/blob/913c5bf4ed323c936836d410d06370fecace90d4/atom_batch_execution/experiments/rrbot_example/template.yml.j2#L39

This removes the unneeded double conversion to integer, and leaves us free to add arbitrary strings as collection keys in the future :) (at least for the batch execution)

Having a look at your branch, you are leaving a lot of debug lines commented. When passing to noetic-devel, please erase them if you are not using them.

I also suggest you to move your experiences away from the softbot_example directory (especially if it does not correspond to the dataset used in atom_examples), and please erase the obtained results from the upstream branch. This is order to keep the atom_batch_execution at a minimum and fully correspondent to the atom_examples. I need input from @miguelriemoliveira here as well, as this is repository organization and ultimately he has the last word here (just like a normal BDFL). I suggest you create a new repository only for your experiences, then you can freely organize the things the way you want to (and update them to GitHub if you want)!

Nevertheless, I believe none of what I said put the results you are taking now in jeopardy, these are only suggestions for future experiences. Keep up the good work and show us good results :)

miguelriemoliveira commented 1 month ago

I think having two of these functions is redundant. To have consistency, I suggest using only the filterCollectionsFromDataset. It also does not make the assumption that collection keys are "converted" integers, as the one you created does. This is more of a code organization issue, so I will ask for the option of @miguelriemoliveira about this specific problem.

I agree. We should always use functions that already exist.

I need input from @miguelriemoliveira here as well, as this is repository organization Also agree.

I suggest you create a new repository only for your experiences, then you can freely organize the things the way you want to (and update them to GitHub if you want)!

Why a repository? It has no code, right? Why not a folder in experiences of the ATOM NAS?

manuelgitgomes commented 1 month ago

Why a repository? It has no code, right? Why not a folder in experiences of the ATOM NAS?

Yes, good idea!

miguelriemoliveira commented 1 month ago

I looked and there is already a softbot folder in the experiments NAS directory.

brunofavs commented 1 month ago

Hi @manuelgitgomes and @miguelriemoliveira ,

You are adding a new field to a complex system that will make all the old templates outdated.

I actually didn't add the collections_to_remove field in the data.yml. It was already there before in the initial draft @miguelriemoliveira did but was not doing anything. I do agree that it makes the old templates outdated. Nevertheless I can make it backwards compatible and will do soon.


We already have a script that removes collections from a dataset:

I did think there was a script for this somewhere but I didn't found it yesterday so that was my bad. But this script seems like a nuclear solution. Yeah here it would be appropriate since I don't want those collections anywhere but say the user wanted to try and fiddle around with different parts or different collections of a dataset it would be much easier to just use the argument in the data.yml than creating a new dataset off of the original one.

Also this remove_collections_from_atom_dataset script is very confusing to me rn. There's this filterDatasetByCollection function defined in the beginning that that is not even being used. Moreover this filterDatasetByCollection is defined in multiple files throughout the project and is only actually used here :

https://github.com/lardemua/atom/blob/5de84fc37d92d9f1bf7b260335d8d2079a1936a3/atom_calibration/scripts/utilities/split_atom_dataset#L75


It also does not make the assumption that collection keys are "converted" integers, as the one you created does.

That is because this function iterates the collection list looking for any that match some criteria. It doesn't have the functionality to directly remove a certain collection. Only with the lambda -csf but that requires the argeparse from the calibrate script.

I might be confusing but seems that you were telling me NOT to delete those collections in the batch execution script before and now you are telling me to use the filterCollectionsFromDataset function istead?

Using the filterCollectionsFromDataset seems like over adapating a function taylored for the calibrate script to use here to accomplish a simple task. I would have to add a argument to the batch execution named -csf just for this. I don't see the value in this approach. The experiment information would be scattered across both the data.yml and the script's argeparse.


dataset_corrected.json is not a mandatory part of the ATOM pipeline, so it should not be necessary to use in the batch execution. The cross validator relies on the detected parameter of the labels. Even if you change the labeling, I believe this parameter is not changed most of the times. I agree that in some situations it might be necessary. However, I suggest you add a flag or some other way to use dataset_corrected.json optionally, while using dataset.json as a default.

Here you are right, a optional argument does make much more sense, as the corrected json isn't in fact necessarily a part of the pipeline


I am also changing the example templates to remove collection conversion to integer:

It is more concise indeed. I would like to bring up the fact everywhere else on the project when lambdas are used it it mandatory to use the conversion to integer int(x), while here from now on it won't. This doesn't impact anything, just might catch a outside user offguard. But its fine by me if it works the same way.


Having a look at your branch, you are leaving a lot of debug lines commented. When passing to noetic-devel, please erase them if you are not using them.

This is also true, thank you for reminding me.


I also suggest you to move your experiences away from the softbot_example directory (especially if it does not correspond to the dataset used in atom_examples), and please erase the obtained results from the upstream branch. This is order to keep the atom_batch_execution at a minimum and fully correspondent to the atom_examples. I need input from @miguelriemoliveira here as well, as this is repository organization and ultimately he has the last word here (just like a normal BDFL). I suggest you create a new repository only for your experiences, then you can freely organize the things the way you want to (and update them to GitHub if you want)!

This is also fine by me. The previous example that was on the softbot's folder was outdated and incomplete, so we'll go from having a very complex example to not having one at all.

What I can do that might be better is : now meanwhile I'm doing a lot of changes and would like to have source control and whatnot, I keep it here in atom. Then when its finished I copy it to the NAS and working I strip it down to the bare minimum and we have a working simple softbot batch experiment example.

brunofavs commented 1 month ago

Also these experiences are taking toooo much time.

It's been running for 16 hours and it's like 70% done. There is going to be a lot of data atleast.

Also I looked roughly at some experiences and the results do look great actually, so thats a good thing.

miguelriemoliveira commented 1 month ago

I actually didn't add the collections_to_remove field in the data.yml. It was already there before in the initial draft @miguelriemoliveira did but was not doing anything. I do agree that it makes the old templates outdated. Nevertheless I can make it backwards compatible and will do soon.

I remember now this was in the template yes, so we can remove some collections.

I did think there was a script for this somewhere but I didn't found it yesterday so that was my bad. But this script seems like a nuclear solution. Yeah here it would be appropriate since I don't want those collections anywhere but say the user wanted to try and fiddle around with different parts or different collections of a dataset it would be much easier to just use the argument in the data.yml than creating a new dataset off of the original one.

I also think it would be nice to remove collections on the fly, as we do with the csf flag. Actually, the collections_to_remove in the yaml is just to set the -csf flag, if I remember correctly.

What I can do that might be better is : now meanwhile I'm doing a lot of changes and would like to have source control and whatnot, I keep it here in atom. Then when its finished I copy it to the NAS and working I strip it down to the bare minimum and we have a working simple softbot batch experiment example.

I do not agree with this one because it never goes as planned. Might as well start using the solution now, otherwise we'll forget to do it in the future. So if you can, please, use the experiments in the NAS from now.

Also I looked roughly at some experiences and the results do look great actually, so thats a good thing.

Great news!

brunofavs commented 1 month ago

Hi @miguelriemoliveira

I do not agree with this one because it never goes as planned. Might as well start using the solution now, otherwise we'll forget to do it in the future. So if you can, please, use the experiments in the NAS from now.

Ok when these tests finish running I'll move the things over to the NAS then.


I also think it would be nice to remove collections on the fly, as we do with the csf flag. Actually, the collections_to_remove in the yaml is just to set the -csf flag, if I remember correctly.

Yeah this was my first approach as well. The downside is that the if we use the collections_to_remove to set the jinja variable in the template, with something like ..

rosrun calibrate ....... -csf 'lambda x : x in {{ fold[0] }} and x not in {{ collections_to_remove}} the collections to remove still end up in the fold list and the code is more innefficient.

The way I did the collections were removed before the cross validator class generator so that the generated folds would only include the collections we actually wanted and the templates could be simplified to just something like :

rosrun calibrate ....... -csf 'lambda x : x in {{ fold[0] }}

This ensures we always have the same number of collections per run, whilst the former method does not.

manuelgitgomes commented 1 month ago

Hello to both,

I actually didn't add the collections_to_remove field in the data.yml. It was already there before in the initial draft @miguelriemoliveira did but was not doing anything. I do agree that it makes the old templates outdated. Nevertheless I can make it backwards compatible and will do soon.

I did not know that, thank you!


Yeah here it would be appropriate since I don't want those collections anywhere but say the user wanted to try and fiddle around with different parts or different collections of a dataset it would be much easier to just use the argument in the data.yml than creating a new dataset off of the original one.

Ok, it makes sense to have it, I agree now.


Also this remove_collections_from_atom_dataset script is very confusing to me rn. There's this filterDatasetByCollection function defined in the beginning that that is not even being used. Moreover this filterDatasetByCollection is defined in multiple files throughout the project and is only actually used here :

Yes, they are not well organized. Feel free to clean those scripts :D


I might be confusing but seems that you were telling me NOT to delete those collections in the batch execution script before and now you are telling me to use the filterCollectionsFromDataset function istead?

Basically, I was saying that the collection deletion should not be in the batch execution (which I do not agree now), but if it was, I think it should be done in another way than the way it is implemented now.

Using the filterCollectionsFromDataset seems like over adapating a function taylored for the calibrate script to use here to accomplish a simple task.

The thing is that this function is used everywhere else in ATOM.

The way I did the collections were removed before the cross validator class generator so that the generated folds would only include the collections we actually wanted and the templates could be simplified

I agree with that, but you could do it using the filterCollectionsFromDataset, without the need of creating a new function. Just create a dictionary args or similar that incorporates the collection_selection_function, etc... This way it would prevent a new function with the same functionality, albeit more specific, to be created.


I would like to bring up the fact everywhere else on the project when lambdas are used it it mandatory to use the conversion to integer int(x), while here from now on it won't.

Is it? I believe recommended, but not mandatory. When manually typing the -csf, you can write something like lambda x: x not in ["000", "056", "078"]. It is more cumbersome to do it this way, but it is also possible.


Also I looked roughly at some experiences and the results do look great actually, so thats a good thing.

That's great :)

brunofavs commented 1 month ago

Hey @miguelriemoliveira @manuelgitgomes

The example is on NAS now, all the results along with the processed ones too.


I agree with that, but you could do it using the filterCollectionsFromDataset, without the need of creating a new function. Just create a dictionary args or similar that incorporates the collection_selection_function, etc... This way it would prevent a new function with the same functionality, albeit more specific, to be created.

I reworked it to use the filterCollectionsFromDataset and it still works great. It works slightly differently in the sense that now it auto-excludes the incomplete collections. This isn't a problem because it just excluded 2 collections I was already excluding by myself manually anyway.


However, I suggest you add a flag or some other way to use dataset_corrected.json optionally, while using dataset.json as a default.

Now it searches the dataset path to check if the corrected json exists, uses it if so, otherwise uses the normal dataset.json.

Is it? I believe recommended, but not mandatory. When manually typing the -csf, you can write something like lambda x: x not in ["000", "056", "078"]. It is more cumbersome to do it this way, but it is also possible.

Yeah I guess you are right.

brunofavs commented 1 month ago

Other thing @manuelgitgomes.

I'm not sure what is going on because I just pulled your changes ( I forgot before) and it's throwing some errors that I'm not quite aware what they are. Im temporarily reverting to the way I had things before.

Traceback (most recent call last):
  File "/home/bruno/atom_ws/src/atom/atom_batch_execution/scripts/batch_execution", line 211, in <module>
    main()
  File "/home/bruno/atom_ws/src/atom/atom_batch_execution/scripts/batch_execution", line 134, in main
    folds = list(folds)
  File "/usr/local/lib/python3.8/dist-packages/sklearn/model_selection/_split.py", line 370, in split
    raise ValueError(
ValueError: Cannot have number of splits n_splits=5 greater than the number of samples: n_samples=1.
brunofavs commented 1 month ago

I reverted your commits 913c5bf4ed32 and 70a04b33f2c8 and it's still giving the same error. I'm beyond confused now

manuelgitgomes commented 1 month ago

I reverted your commits 913c5bf and 70a04b3 and it's still giving the same error. I'm beyond confused now

It seems completely non-related to my changes. Did you remove the correct number of collections? The error basically says that you have a dataset with a single collection.

brunofavs commented 1 month ago

It was working flawlessly before I merged your changes, I'll dig a bit more, might be a simple thing

brunofavs commented 1 month ago

It was working flawlessly before I merged your changes, I'll dig a bit more, might be a simple thing

Nah it wasn't, I was indeed removing everything. I was being dumb.

brunofavs commented 1 month ago

It's fixed now

brunofavs commented 1 month ago

@miguelriemoliveira Do you want to meet sometime soon to discuss the results?

I think they are good and most cases have been tackled, but there might be some other things we maybe should experiment as well that I might not be seeing.

Also I need some small clarifications regarding the experiments with zau. I'll work on preparing the template for zau now. Should be pretty easy now that I have the hang of it.

You are definitely welcome to join too @manuelgitgomes if we meet :)

miguelriemoliveira commented 1 month ago

However, I suggest you add a flag or some other way to use dataset_corrected.json optionally, while using dataset.json as a default.

Now it searches the dataset path to check if the corrected json exists, uses it if so, otherwise uses the normal dataset.json.

Bu why don't we use a -json argument like we do everywhere else in ATOM? I would be more consistent ...

Do you want to meet sometime soon to discuss the results?

We can meet tomorrow at 9h?

miguelriemoliveira commented 1 month ago

I mean in zoom at 9h, or if you prefer, we can meet at 11h in LAR.

brunofavs commented 1 month ago

I do prefer LAR by 11h. I always prefer in person as a matter of fact.

brunofavs commented 1 month ago

Bu why don't we use a -json argument like we do everywhere else in ATOM? I would be more consistent ...

I would argue against. Everywhere else in ATOM we use the -json argument to define which dataset we are using. That is absolutely true. However here the dataset is already defined in the data.yml so we would be defining it twice. Also in the cases referred initially, those arguments impact the script in question immensely, like choosing which dataset to do the calibration with. Here is really just a minor optimization that won't make a big difference 95% of the time. Odds are a user would try the flag see no difference and be confused.

We could remove it from the data.yml and have it only on the argument. That would be more consistent with the other scripts and we would only be defining the dataset path once. Unfortunately that would require a modest rework to the script.

miguelriemoliveira commented 1 month ago

However here the dataset is already defined in the data.yml

Ah, ok. So if its defined there, why can't we say in the yaml if its the dataset.json or the dataset_corrected.json?

miguelriemoliveira commented 1 month ago

Just to say. I accept some of the way stuff is done in ATOM may not be the best. But we should always consider consistency as an important thing.

Like that conversation we had about your idea to setup the colorama reset.

Until 11h.

brunofavs commented 1 month ago

We can I think. But maybe we'd need some further modifications in the code as it relies on the dataset's directory name but should be feasible.

brunofavs commented 1 month ago

Just to say. I accept some of the way stuff is done in ATOM may not be the best. But we should always consider consistency as an important thing.

Like that conversation we had about your idea to setup the colorama reset.

Until 11h.

Absolutely. If it wasn't for ATOM's consistency I would take way longer to understand and implement features. Very often I know just where to look for things I have never messed with because of said consistency and how those things might work as well.

miguelriemoliveira commented 1 month ago

Absolutely. If it wasn't for ATOM's consistency I would take way longer to understand and implement features. Very often I know just where to look for things I have never messed with because of said consistency and how those things might work as well.

Right. See @manuelgitgomes and @Kazadhum ?

@brunofavs also does not want to use numpy arrays :-)

brunofavs commented 1 month ago

We can I think. But maybe we'd need some further modifications in the code as it relies on the dataset's directory name but should be feasible.

Was actually the simplest of fixes. be9f9bf

brunofavs commented 1 month ago

@brunofavs also does not want to use numpy arrays :-)

For me if a task envolves large computing resources and matrix operations np arrays make the most sense. Otherwise I much rather use lists as they are native python objects and have far less constrictions