MonteCarlo packet progress bar completes to 100%

[shreyas3156]

📝 Description

Type: 🪲 bugfix

The Monte-Carlo packet progress bar used to update during every iteration but did not complete to 100% once it was over. An update call needed to be included at the end of each iteration.

resolves #2216

📌 Resources

tqdm Docs

🚦 Testing

How did you test these changes?

• Testing pipeline
• Other method (describe)
• My changes can't be tested (explain why)

Before:

After:

☑️ Checklist

• I requested two reviewers for this pull request
• I updated the documentation according to my changes
• I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

[tardis-bot]

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

[sonachitchyan]

Thanks for the PR! Please make sure to runblack on the files that you have modified.

[shreyas3156]

Thanks for the PR! Please make sure to runblack on the files that you have modified.

@sonachitchyan Thank you for the quick review! I have made the changes after running it.

[codecov]

Codecov Report

Merging #2237 (0579332) into master (239e04b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0579332 differs from pull request most recent head 348994a. Consider uploading reports for the commit 348994a to get more accurate results

@@           Coverage Diff           @@
##           master    #2237   +/-   ##
=======================================
  Coverage   71.81%   71.82%           
=======================================
  Files         133      133           
  Lines       12355    12358    +3     
=======================================
+ Hits         8873     8876    +3     
  Misses       3482     3482           

Impacted Files	Coverage Δ
tardis/montecarlo/montecarlo_numba/base.py	32.50% <100.00%> (+0.56%)	⬆️
tardis/util/base.py	74.20% <100.00%> (+0.20%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sonachitchyan]

Looks good to me!

[DhruvSondhi]

This is maybe a better approach. I am unsure what you are doing (which is like, in the end, when all the iterations have finished, you are refreshing the progress bar) and if this is a good idea. Try this, as it refreshes the progress bar after each packet. And thank you for contributing to TARDIS :)

[DhruvSondhi]

I suggest not making a function altogether here. Doing the usual packet_pbar.refresh() may be better. You may need to check where exactly this can be referenced and changed but doing so would make it easier and not complex.

[shreyas3156]

I agree that refreshing the progress bar after every packet makes the most logical sense.

However, the bar is indeed updated automatically after every packet is sent. The issue arises at the end of any given iteration when packet_pbar.update() is called to update the packet but the change is not reflected. This can be solved by refreshing the bar at the end of every iteration (not just the final one).

[DhruvSondhi]

Moving this inside the above function may do the trick. Please let me know if that works. Also add a single line comment for the same instead of having a function.

[shreyas3156]

Since the update/refresh does not occur only at the end of each iteration, I decided to call it when the iteration ends. Adding the packet_pbar.refresh() at every packet would increase the simulation time severely (around 1 min/iteration) in contrast to refreshing only at the end (70s for 20 iterations on my system).

This is why I could not find a suitable place to add the refresh anywhere in the function above (in update_packet_pbar()). Also because one cannot conclude when the final packet is sent (probably because it is numba jitted in parallel mode).

Thus, the only remaining solution was to either add the packet_bar.refresh() within the update_iterations_pbar() function as it is called at the end of every iteration, or have a function of its own.

[DhruvSondhi]

Thus, the only remaining solution was to either add the packet_bar.refresh() within the update_iterations_pbar() function as it is called at the end of every iteration, or have a function of its own.

I suppose it is better to have just this line packet_bar.refresh() after the update_iterations_pbar(1) as having a function just for this is not good, in my opinion, and is just overhead with a call within a functional call.

[shreyas3156]

I suppose it is better to have just this line packet_bar.refresh() after the update_iterations_pbar(1) as having a function just for this is not good, in my opinion, and is just overhead with a call within a functional call.

The packet_pbar and iterations_pbar instances of the progress bars are initialized and processed in tardis/utility/base.py. So, packet_bar.refresh() after update_iterations_pbar(1) would require instantiating the progress bars in montecarlo_numba/base.py as well, which may not be the best approach.

I agree that it is an avoidable overhead, in which case I can just add it in the update_iterations_pbar() method after iterations_pbar.update(i). It would look something like:

def update_iterations_pbar(i):
    iterations_pbar.update(i)
    packet_bar.refresh()

[DhruvSondhi]

This would be inefficient as the current implementation updates the progress bar by 1 per cent after a specific increase in the progress as the packets are propagated. What this may do is that after every 1 per cent, the progress bar will get refreshed and updated. For this, I suppose you can look into making something like this: (pseudo code here)

If the progress is above, let's say, 95% (it would be good if this is as close to 100 as possible or maybe even 100):
(This could also be by iteration number, like after iteration, maybe 19 (if we are running for 20 iterations))
  Refresh the bar for each iteration until it reaches the end,
Else:
  Keep doing what it initially was doing,

What happens with the numba jitted functions is that the information is present with the compiler (machine level compiler) but is not actually passed onto into Python runtime (Python interpreter), which causes loss of information as hence it doesn't understand how to add all the multithreaded information together from different cores.

Another possible solution could be to update the progress bar by returning the instance of the progress bar from the update_iterations_pbar() function and then updating it when all the progress is finished. What I mean by this is that even when running TARDIS in multithreaded mode, the progress bar considers all the progress made on all the cores. And in the end, please correct me; it is still compiled on a single core. So a single call for refreshing the progress bar at the end after completing all the propagated packets may do the trick. It works for all other iterations except the last one. This would be more efficient.

[shreyas3156]

This would be inefficient as the current implementation updates the progress bar by 1 per cent after a specific increase in the progress as the packets are propagated. What this may do is that after every 1 per cent, the progress bar will get refreshed and updated.

The update_iterations_pbar() is called to update the iterations progress bar (iterations_pbar) at the end of each iteration, only after the packet progress bar (packet_pbar) has finished updating for that iteration, i.e., after all the packets for that iteration have been sent. So I think this refresh would not interfere with the packet_pbar while the packets are being sent and only update it after sending 100% of the packets.

What happens with the numba jitted functions is that the information is present with the compiler (machine level compiler) but is not actually passed onto into Python runtime (Python interpreter), which causes loss of information as hence it doesn't understand how to add all the multithreaded information together from different cores.

Thank you for the clear description. This may be why there is no possible way to fetch the completion percentage as the number of packets sent through different cores is not trackable.

So a single call for refreshing the progress bar at the end after the completion of all the propagated packets may do the trick.

My intention with the code block mentioned in the previous reply was precisely this. Since update_iterations_pbar() is called at the end of every iteration, adding the refresh part within it would also be subject to a call at the end of each iteration when all the packets have propagated. Please correct me if I've misunderstood.