Skip to content

JAX: Add JAX implementation for wealth dynamics#304

Merged
jstac merged 11 commits into
mainfrom
jax_wealth_dynamics_1
Jan 17, 2023
Merged

JAX: Add JAX implementation for wealth dynamics#304
jstac merged 11 commits into
mainfrom
jax_wealth_dynamics_1

Conversation

@Smit-create

Copy link
Copy Markdown
Member

@github-actions

github-actions Bot commented Dec 30, 2022

Copy link
Copy Markdown

@Smit-create Smit-create changed the title JAX: Add JAX implementation for wealth dynamics #303 JAX: Add JAX implementation for wealth dynamics Dec 31, 2022
@Smit-create

Copy link
Copy Markdown
Member Author

Hi @HumphreyYang ,

This seems to be ready for the first review. Please have a look. Thanks

@Smit-create
Smit-create marked this pull request as ready for review December 31, 2022 06:15

@HumphreyYang HumphreyYang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many Thanks for the update @Smit-create. The code looks great to me. One improvement we could potentially make is to hide the code using numba using

---
tags: [hide-input]
---

and only compare the speed when making the final comparison.

I think it is clear to readers when comparing the performance of two implementations in my case since there is only one function involved, but in this case, having two implementations throughout the lecture may confuse readers and obscure the focus of the lecture. Please let me know what you think :) (also CC @jstac)

Comment on lines +89 to +92
# Check if JAX is using GPU
print("jax backend: {}".format(jax.lib.xla_bridge.get_backend().platform))
# Check the devices available for JAX
print(jax.devices())

@HumphreyYang HumphreyYang Jan 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use

print(f"jax backend: {jax.devices()[0].platform}")

in our PRs for simplicity and consistency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HumphreyYang , simple is good.

@Smit-create , please change jax to JAX in the text.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will change that accordingly.

@Smit-create

Copy link
Copy Markdown
Member Author

Please let me know what you think :)

This sounds like a nice idea, let's try that out and see how it looks.

@jstac

jstac commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

@Smit-create and @HumphreyYang, please let me know when this is ready for another review.

@HumphreyYang

Copy link
Copy Markdown
Member

Please let me know what you think :)

This sounds like a nice idea, let's try that out and see how it looks.

@Smit-create I think it looks cleaner and simpler with Numba code hidden. How do you feel about this change?

@Smit-create

Copy link
Copy Markdown
Member Author

How do you feel about this change?

I think that looks cleaner. What do you think @jstac?

please let me know when this is ready for another review.

@jstac, I think that the content is ready for review. Might just need to edit/add a couple of cells following the discussions in #301 (comment).

Comment thread lectures/wealth_dynamics.md Outdated
## Implementation
## Implementation using JAX

Let's define a Model to represent the wealth dynamics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change "Model" to "model" and the same below.

Comment thread lectures/wealth_dynamics.md Outdated
update_states_jax = jax.jit(update_states_jax, static_argnums=(2,))
```

Here’s function to simulate the time series of wealth for individual households using `for` loop and JAX.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using a for loop


# 2nd time is expected to be very fast because of JIT
w_jax_result = wealth_time_series_for_loop_jax(wdy.y_mean, ts_length, wdy, size)
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need block_until_ready?

I guess not but just checking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add that. The documentation for benchmarking also recommends using it: https://jax.readthedocs.io/en/latest/faq.html#benchmarking-jax-code

@jstac

jstac commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

Looking at the current deployment https://63b52771da28e92bd5e7733b--nostalgic-wright-5fa355.netlify.app/wealth_dynamics.html shows that one numba code cell is not hidden, while the others are. Is this intensional?

@jstac

jstac commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

Should we compare the timing of update_cross_section_jax and update_cross_section?

I suspect that, on a large cross-section, the JAX code will be much faster...

@jstac

jstac commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

Does this sentence need to be adjusted/removed?

"Notice how effectively Numba has implemented multithreading for this routine: all 8 CPUs on our workstation are running at maximum capacity (even though four of them are virtual)."

I think we are using JAX at this point.

@jstac

jstac commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

Many thanks @Smit-create . Please see my comments above.

@mmcky , if you have time please have a quick glance at this once @Smit-create has pushed changes.

@Smit-create

Copy link
Copy Markdown
Member Author

shows that one numba code cell is not hidden, while the others are. Is this intentional?

I thought to keep it unhidden so that the below cell that measures time can relate to it. But let's hide all the numba code and just keep the JAX available readily.

Should we compare the timing of update_cross_section_jax and update_cross_section?

It's already been compared using generate_lorenz_and_gini_jax (while plotting) and shows about a ~30s difference.

"Notice how effectively Numba has implemented multithreading for this routine: all 8 CPUs on our workstation are running at maximum capacity (even though four of them are virtual)."

Let's remove this completely along with the image?

@mmcky

mmcky commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

thanks @Smit-create, @HumphreyYang and @jstac

Is the purpose of hiding the Numba code to clean up the lecture and enable those interested to see the code. It is not the approach often taken relative to our other lectures which tends to always show the code but I see the application here.

This is what it looks like in the preview

Screenshot 2023-01-13 at 1 05 33 pm

@mmcky

mmcky commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

For the text in the lecture

Notice the time difference between the wealth_time_series and wealth_time_series_jax

Could we use the glu feature to have the values here? It is a statement that requires the reader to find the values and compare them. https://jupyterbook.org/en/stable/reference/cheatsheet.html?highlight=glu#gluing-numbers

@jstac

jstac commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Yes, I think it looks nice to have the numba code hidden, and I can understand the motivation, but at the same time it might be confusing for the reader --- why is some code hidden and some not?

Also, if we hide numba code here, then we should do it elsewhere when we use JAX --- but then it's a burden and possibly unclear exactly what we should hide.

Having thought all this through, I suggest that we show the numba code rather than hiding it and do the same for the remaining lectures.

@Smit-create

Copy link
Copy Markdown
Member Author

I suggest that we show the numba code rather than hiding it and do the same for the remaining lectures.

Sure, will do that.

Could we use the glue feature to have the values here?

Yes, added it, thanks!

@jstac

jstac commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Thanks @Smit-create

@HumphreyYang just checking that you saw the discussion above regarding hiding / not hiding.

Could you please give this PR one more careful look over and then let me know when you are done. After that I think we'll be ready to merge.

@HumphreyYang

Copy link
Copy Markdown
Member

Thanks @Smit-create

@HumphreyYang just checking that you saw the discussion above regarding hiding / not hiding.

Could you please give this PR one more careful look over and then let me know when you are done. After that I think we'll be ready to merge.

Hi @jstac,

Before that, could you please have a look at the GPU Warning and JAX imports I pushed in my PR so that @Smit-create and I can have a consistent GPU warning?

@jstac

jstac commented Jan 14, 2023

Copy link
Copy Markdown
Contributor

Thanks @HumphreyYang , good idea. Here's what I suggest for the GPU warning, which slightly modifies the version of @HumphreyYang :

This lecture is built using hardware that has access to a GPU and uses JAX for GPU programming. As a result, the lecture will be slower when running on a machine without a GPU.

Free GPUs are available on Google Colab. To use this option, please click on the play icon top right, select Colab, and set the runtime environment to include a GPU.

Alternatively, if if you have your own GPU, you can follow the instructions for for installing JAX with GPU support.

@Smit-create

Copy link
Copy Markdown
Member Author

The PR is ready now. The failure is because of this diff. Once we merge that PR, I'll rebase this on top of that and that should fix the CI.

@jstac

jstac commented Jan 16, 2023

Copy link
Copy Markdown
Contributor

Thanks @Smit-create

@Smit-create

Copy link
Copy Markdown
Member Author
  • Please see the requested changes to GPU Warning

Thanks for the review @jstac. I have updated the diff.

Is there a working netlify deployment I can look at here?

The last commit (10th commit) was failing on CI and so I suppose it won't be deployed. I have pushed a new commit and let's see if that works.

@Smit-create

Copy link
Copy Markdown
Member Author

The tests pass and the deployed version is available at https://63c622d321fc755cc8652567--nostalgic-wright-5fa355.netlify.app/

@jstac

jstac commented Jan 17, 2023

Copy link
Copy Markdown
Contributor

Great work @Smit-create , many thanks. Merging.

@jstac
jstac merged commit 5f7ea92 into main Jan 17, 2023
@jstac
jstac deleted the jax_wealth_dynamics_1 branch January 17, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants