Skip to content

Conversation

@xuanguang-li
Copy link
Contributor

@xuanguang-li xuanguang-li commented Aug 12, 2025

Key changes

  • Set the max-height of cells to none at the beginning
  • Removed object after class Nuemann
  • Adopted sphinx-proof environments

Question

  • As I used the sphinx-proof, assumptions numbered as "I, II" have been changed to "1, 2". But in the definition of class Neumann, some variables are named as AI and AII. Should I change the names of these variables to A1 and A2? @mmcky

For example,

def __init__(self, A, B):

        self.A, self.B = list(map(self.convert, (A, B)))
        self.m, self.n = self.A.shape

        # Check if (A, B) satisfy the basic assumptions
        assert self.A.shape == self.B.shape, 'The input and output matrices \
              must have the same dimensions!'
        assert (self.A >= 0).all() and (self.B >= 0).all(), 'The input and \
              output matrices must have only non-negative entries!'

        # (1) Check whether Assumption 1 is satisfied:
        if (np.sum(B, 0) <= 0).any():
            self.AI = False
        else:
            self.AI = True

        # (2) Check whether Assumption 2 is satisfied:
        if (np.sum(A, 1) <= 0).any():
            self.AII = False
        else:
            self.AII = True

linked to #478

- Set the max-height of cells to none
- Removed `object` after `class Nuemann`
- Adopted `sphinx-proof` enviornments
@github-actions
Copy link

github-actions bot commented Aug 12, 2025

@github-actions github-actions bot temporarily deployed to pull request August 12, 2025 10:24 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 12, 2025 10:25 Inactive
@mmcky mmcky requested a review from Copilot August 12, 2025 10:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the von Neumann lecture by modernizing documentation formatting and fixing minor inconsistencies. The changes primarily focus on adopting sphinx-proof environments for better mathematical presentation and updating code cell configurations.

Key changes:

  • Adoption of sphinx-proof environments for assumptions, definitions, theorems, and proofs
  • Removal of object inheritance from class Neumann (Python 3 style)
  • Addition of CSS styling to remove cell height restrictions

Assumptions:
- AI: every column of B has a positive entry : {AI}
- AII: every row of A has a positive entry : {AII}
Assumptions:// TODO:change {AI} variable into A1?
Copy link

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

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

The TODO comment should be removed or properly formatted. If this is a genuine TODO, it should be tracked in an issue rather than left in production code.

Suggested change
Assumptions:// TODO:change {AI} variable into A1?
Assumptions:

Copilot uses AI. Check for mistakes.
Two key assumptions restrict economy $(A,B)$:

- **Assumption I:** (every good that is consumed is also produced)
````{prf:assumption} every good that is consumed is also produced)
Copy link

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

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

There is an extra closing parenthesis in the assumption title. It should be 'every good that is consumed is also produced' without the trailing parenthesis.

Suggested change
````{prf:assumption} every good that is consumed is also produced)
````{prf:assumption} every good that is consumed is also produced

Copilot uses AI. Check for mistakes.
@mmcky mmcky self-requested a review August 12, 2025 11:03
@mmcky
Copy link
Contributor

mmcky commented Aug 12, 2025

Thanks for the PR @xuanguang-li. I will take a look tomorrow.

@mmcky
Copy link
Contributor

mmcky commented Aug 14, 2025

@jstac -- @xuanguang-li has asked a really nice style clarification question.

In the case where there are two or more conditions are states, should a python variable name always use numbers such as A1, A2?

What makes me pause is that in some documents Assumption admonitions can be roman numerals in style and in fact the docstring associated with this code uses roman numerals so it may be "closer to the description" keeping the roman numerals in the code.

    Both A and B have non-negative entries. Moreover, we assume that
    (1) Assumption I (every good which is consumed is also produced):
        for all j, b_{.,j} > 0, i.e. at least one entry is strictly positive
    (2) Assumption II (no free lunch):
        for all i, a_{i,.} > 0, i.e. at least one entry is strictly positive

@jstac
Copy link
Contributor

jstac commented Aug 14, 2025

so it may be "closer to the description" keeping the roman numerals in the code.

That sounds like good reasoning to me @mmcky .

@xuanguang-li
Copy link
Contributor Author

Hi John and @mmcky,

Thanks for your feedback.

I understand your point that I should keep the variable names consistent with the docstrings. Should I still use sphinx-proof directives to write assumptions?

```{prf:assumption} every good that is consumed is also produced
:label: assumption1

$$
  b_{.,j} > \mathbf{0}\hspace{5mm}\forall j=1,2,\dots,n
$$

If I use sphinx-proof directives, Assumption admonitions will be numbered as Assumption 1 rather than in Roman numerals, as shown in the attached pictures. The reference of this assumption using {prf: ref} will also display as Assumption 1.

image image

@mmcky
Copy link
Contributor

mmcky commented Aug 14, 2025

thanks @xuanguang-li let me think this through, I take your point that we will have mixed systems in the lecture 👍

One option would be to enable roman numeral styles for sphinx-proof.

 - Kept roman numerals in `class nuemann`.
- Deleted `<style>` setting which does not work
@xuanguang-li
Copy link
Contributor Author

Thanks for your explanation. I kept the Roman numerals in the code while using the default sphinx-proof setting, as enabling Roman numerals requires editing conf.py.

Regarding the suggestion, "Only part of class Neumann(object): is showing --- show all", I tried adding a <style> tag in this file and creating a custom.css file to remove the cell height restriction, but both attempts failed. It seems I need to edit the _config.yml file instead. Are there other methods I could try, or should I leave this issue? @mmcky

@github-actions github-actions bot temporarily deployed to pull request August 14, 2025 08:24 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 14, 2025 08:24 Inactive
@mmcky
Copy link
Contributor

mmcky commented Aug 14, 2025

thanks @xuanguang-li we will always keep the css in quantecon-book-theme but thanks for giving that a go.

The collapse-20 changes the code cell to be only 20 lines wide and then adds a scroll, but I agree with you it makes it very hard to read the code.

  • review style of collapse-## in the quantecon-book-theme as this has changed (perhaps downstream in sphinx_book_theme. It should expand out and collapse (as we had before).

@xuanguang-li can you make a new issue based on this issue for tracking? I would suggest using collapse-40 for now perhaps.

@xuanguang-li
Copy link
Contributor Author

Thanks @mmcky. I have created a new issue QuantEcon/quantecon-book-theme#299 describing this problem.

@xuanguang-li xuanguang-li marked this pull request as ready for review August 15, 2025 01:13
@xuanguang-li
Copy link
Contributor Author

This PR is ready for review. @mmcky @HumphreyYang

@mmcky mmcky requested a review from HumphreyYang August 16, 2025 00:05
output matrices must have only non-negative entries!'
# (1) Check whether Assumption I is satisfied:
# (1) Check whether Assumption 1 is satisfied:
Copy link
Contributor

Choose a reason for hiding this comment

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

@xuanguang-li could you revert this to roman numerals? I think we have kept the code the same, so these should also be roman numeral. Is that right? Thanks.

@mmcky mmcky self-requested a review August 16, 2025 00:07
Copy link
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

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

thanks @xuanguang-li. Just one minor pickup.

@HumphreyYang I think this is in pretty good shape.

Copy link
Member

@HumphreyYang HumphreyYang left a comment

Choose a reason for hiding this comment

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

Many thanks, @xuanguang-li! Great PR!

Just two very minor comments below.

Another small comment is that I think this lecture is not yet style-sheet compliant, as some paragraphs contain more than one sentence : )

relationship between technological and valuation characteristics of
the economy:

**Definition:** The *technological expansion problem* (TEP) for the economy
Copy link
Member

Choose a reason for hiding this comment

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

Here I think we want to close the definition environment after the equation.

image

Co-authored-by: Humphrey Yang <39026988+HumphreyYang@users.noreply.github.com>
@mmcky
Copy link
Contributor

mmcky commented Aug 16, 2025

thanks @HumphreyYang.

@xuanguang-li it would be great if you could read through https://manual.quantecon.org/styleguide/writing.html and see if you make some improvements re: style guide compatibility.

@github-actions github-actions bot temporarily deployed to pull request August 16, 2025 07:52 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 16, 2025 07:53 Inactive
@xuanguang-li
Copy link
Contributor Author

Thanks for the comments @mmcky @HumphreyYang.

I will correct some typos and check the style again with the guidance.

@mmcky
Copy link
Contributor

mmcky commented Aug 17, 2025

thanks for your work on this @xuanguang-li -- this is getting close.

@mmcky mmcky added the content label Aug 18, 2025
@xuanguang-li
Copy link
Contributor Author

Hi @mmcky @HumphreyYang,

I have divided the paragraphs into one sentence and used bold font to emphasize the definitions.

Do you have any suggestions or further improvements I could make?

@github-actions github-actions bot temporarily deployed to pull request August 18, 2025 02:41 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 18, 2025 02:42 Inactive
@mmcky mmcky changed the title von Neumann lecture improvements #478 [von_neumann_model] lecture improvements #478 Aug 18, 2025
@mmcky mmcky changed the title [von_neumann_model] lecture improvements #478 [von_neumann_model] lecture improvements Aug 18, 2025
@mmcky mmcky self-requested a review August 18, 2025 07:10
Copy link
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

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

Thanks @xuanguang-li this looks good to me.

@HumphreyYang when you have time could you do a final parse of the diff.

Merge target date: 26th August 2025.

@HumphreyYang
Copy link
Member

HumphreyYang commented Aug 19, 2025

Many thanks @xuanguang-li and @mmcky! These changes are great improvements!

I just noticed that we might want to change ^T to ^\top according to the style sheet, and some minor typos.

I have collected them in one push above.

Once it builds I will merge if @mmcky agrees

@github-actions github-actions bot temporarily deployed to pull request August 19, 2025 04:51 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 19, 2025 04:51 Inactive
@xuanguang-li
Copy link
Contributor Author

Thanks for your help, @HumphreyYang. I will keep this point in mind next time.

@HumphreyYang
Copy link
Member

HumphreyYang commented Aug 19, 2025

Hi @xuanguang-li, I think from previous conversation, we want collapse-40 instead of collapse-20 to be there. Please let me know if I missed anything : )

@xuanguang-li
Copy link
Contributor Author

Yes, generally it is. @HumphreyYang

But in issue #478, John says he wants to show all the code of class Nuemann.

Only part of class Neumann(object): is showing --- show all

@HumphreyYang
Copy link
Member

Yes, generally it is. @HumphreyYang

But in issue #478, John says he wants to show all the code of class Nuemann.

Only part of class Neumann(object): is showing --- show all

Many thanks @xuanguang-li, roger that!

@github-actions github-actions bot temporarily deployed to pull request August 19, 2025 06:43 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 19, 2025 06:44 Inactive
@mmcky
Copy link
Contributor

mmcky commented Aug 20, 2025

@xuanguang-li as Humphrey suggests, let's show all for now (i.e. remove the collapse tag). I have an issue to look into the old behaviour where we could expand and show-all rather than scroll.

@xuanguang-li
Copy link
Contributor Author

Got it, @mmcky! I'll drop the collapse-## tag when I meet it next time.

@HumphreyYang
Copy link
Member

Many thanks @xuanguang-li and @mmcky! Merging it now!

@HumphreyYang HumphreyYang merged commit 677d94b into main Aug 20, 2025
7 checks passed
@HumphreyYang HumphreyYang deleted the von-nuemann branch August 20, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants