Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to documentation & new logger. #210

Merged

Conversation

A-Kibats
Copy link

@A-Kibats A-Kibats commented Jul 4, 2024

This a fixed version of request #197

This pull request focuses on expanding the current docstring, converting existing ones to the NumPy format and changing the Sphinx configuration to accept the format as well as updating the theme to "PyData".

Additionally the current logging system has been changed to use Loguru, files missing logging have been also updated to include it.

None of the core functionality has been changed this is more a fresh coat of paint.

All feedback is welcome especially if something has been missed.

Note

  1. Not all functions & classes have docstrings added.
  2. The formatting for the logs requires feedback to get it's final form.
  3. Docstrings, images and general sphinx settings may need adjusting to make docs look good through HTML.

PyData Theme

Loguru

@A-Kibats
Copy link
Author

A-Kibats commented Jul 4, 2024

In reference to comments left on #197:

Thankyou for the previous feedback!

  1. Yes I agree the figures will need adjusting to fit the theme, with regards to it also fitting the dark mode I think the proper way to go about it would be be to use a custom CSS stylesheet as shown here.

  2. The logo appears to be a .png, a .svg version is being sourced currently, hopefully it should fix the small size. (It may also be a good idea to convert any figures that are not .svg also).

  3. Currently the way the theme has been implemented is extremely bare bones, there's many options and improvements to be made. Some examples of the different web components through Sphinx and of the PyData Theme.

  4. Loguru and docker have been added to the FABulous/requirments.txt, the requirement for the theme and removal of version numbers has been also changed in docs/requirements.txt. Hopefully this should work and cause no issues.

I'll ask @KelvinChung2000 the priority of this work, it may have to wait for now unless someone else would like to continue it.

@A-Kibats
Copy link
Author

A-Kibats commented Jul 5, 2024

New commit made 0268c54

Logo size:

An .svg version of the FABulous logo was provided, however this didn't provide a quick fix to the issue. I would have to increase the size of the navigation bar by too much to produce legible text. I modified the logo by simply moving the text to the right, in a similar manner to how NetworkX has their logo. I also increased the bar size to 80px tall and the modified logo to 70px tall (with auto aspect ratio for the width). The original logo in both .png & .svg are still in the files should they be required.

Before:
image
After:
image

General formatting:

Many of the pages before had the section navigation side bar taking up a good 1/3 of the page space, this includes on the index pages essentially having two parts of the page doing the same function, I've removed it from pages where I deemed it unnecessary. There is also the option to remove the "on this page" side bar, but I've kept it for now as it provides a link to the page source.

Before:
image

After:
image

Optional:
image

Notes:

An additional change that I think would be good is make all titles and headers follow PascalCase typing convention.

docs/source/figs/FAB_logo_long.svg Outdated Show resolved Hide resolved
FABulous/geometry_generator/port_geometry.py Outdated Show resolved Hide resolved
@IAmMarcelJung
Copy link
Collaborator

Thanks for all your changes!

Loguru and docker have been added to the FABulous/requirments.txt, the requirement for the theme and removal of version numbers has been also changed in docs/requirements.txt. Hopefully this should work and cause no issues.

Since we have a dedicated FABulous/docs/requirements.txt, the modules also have to be added there. An alternative would be to remove this dedicated file and just have FABulous/requirements.txt, but I would leave this question to either @KelvinChung2000 or @TaoBi22.

I modified the logo by simply moving the text to the right

I was told that logos are a somewhat political thing, so I'm not sure if this should be done, even though I personally think that it looks good (if the issue mentioned next is resolved). @dirk-koch would know, but he probably does not look here.

Also on my side there seems to be a problem with text overlapping:

image

It looks like this in chrome, firefox and also when viewing the .svg itself with eog. Is this a problem with my machine, since it does look normal in your screenshots?

Many of the pages before had the section navigation side bar taking up a good 1/3 of the page space, this includes on the index pages essentially having two parts of the page doing the same function, I've removed it from pages where I deemed it unnecessary. There is also the option to remove the "on this page" side bar, but I've kept it for now as it provides a link to the page source.

Since the actual content takes up only about 1/3, at maximum 1/2 of the page, I personally don't if the section navigation side takes up a part of the screen where there is not content anyway. However, if the functionality is duplicated in the On this page then on of the two should be removed. But the rest is more of a personal preference i guess :)

An additional change that I think would be good is make all titles and headers follow PascalCase typing convention.

This is a good idea! I totally agree!

@TaoBi22
Copy link
Contributor

TaoBi22 commented Jul 16, 2024

Since we have a dedicated FABulous/docs/requirements.txt, the modules also have to be added there. An alternative would be to remove this dedicated file and just have FABulous/requirements.txt, but I would leave this question to either @KelvinChung2000 or @TaoBi22.

Not up to date on the context here (and feel free to overrule me here as it's been a while since I've had any official involvement with FABulous) but I think having separate requirements files is probably sensible, since the majority of FABulous users won't be wanting to build the docs locally. I've just realised this second file isn't clearly documented anywhere though, I'll add something to the README.

@IAmMarcelJung
Copy link
Collaborator

Since we have a dedicated FABulous/docs/requirements.txt, the modules also have to be added there. An alternative would be to remove this dedicated file and just have FABulous/requirements.txt [...].

#212 makes my statement obsolete, since this makes it clear to just use one venv for both the docs and FABulous itself.

@A-Kibats
Copy link
Author

Hi!

It's my pleasure, I appreciate the feedback!

I was told that logos are a somewhat political thing, so I'm not sure if this should be done, even though I personally think that it looks good (if the issue mentioned next is resolved). @dirk-koch would know, but he probably does not look here.

I was blissfully unaware that it could be a political thing, but fortunately all the original files have been kept in the folders if they do need switching out. The main reason for the change was because it seemed the most straightforward fix for the size issue, without trying to modify the theme dramatically through CSS which i'm not experienced with.

Also on my side there seems to be a problem with text overlapping.

It may have been the way I've generated the .svg, it will definitely need a pass with actual graphical design tools and tweaks. with that however it does look fine on my end with the screenshots taken from chrome and they look the same when opening with MS Edge.

Since the actual content takes up only about 1/3, at maximum 1/2 of the page, I personally don't if the section navigation side takes up a part of the screen where there is not content anyway.

I think if the doc gets expanded and filled out then the navigation side could be useful as it allows for the addition of a search bar, but yes a lot of this is personal preference, we could just take NetworkX as a gold standard as from previous discussion people are in agreement that it looks good.

@KelvinChung2000
Copy link
Collaborator

Modifying the logo in this way I think is acceptable since most companies have vertical and horizontal logos. FABulous will be like the "company" name. I guess the next question to ask is, is the current logo design good enough for presenting what FABulous is about. Can it be used standalone without needing FABulous name on it. We are now doing marketing...

@IAmMarcelJung
Copy link
Collaborator

[...] we could just take NetworkX as a gold standard as from previous discussion people are in agreement that it looks good.

Yeah we should do this :)

It may have been the way I've generated the .svg, it will definitely need a pass with actual graphical design tools and tweaks. with that however it does look fine on my end with the screenshots taken from chrome and they look the same when opening with MS Edge.

Hmm okay, as long as it ends up like this on the actual documentation, it's fine!

I was blissfully unaware that it could be a political thing

I also didn't know this until I was told so recently by @dirk-koch. But if @KelvinChung2000 says it's fine, then it"s fine.

I guess the next question to ask is, is the current logo design good enough for presenting what FABulous is about. Can it be used standalone without needing FABulous name on it. We are now doing marketing...

Yeah this would be a bit too much I guess...

@KelvinChung2000
Copy link
Collaborator

We have not trademarked or copyrighted the name anyway. Does our license mention anything about that? I cannot represent Dirks's opinion; if he dislikes it, we can revert the change and compromise on the look. It has been a while since I've talked to him.

@A-Kibats
Copy link
Author

Hi,

@KelvinChung2000 Kindly provided a better version of the wide logo, the .svg version also started having text overlap so I switched it out for the png version provided, hopefully that should ensure it'll look the same across all machines and browsers.

Before:

Screenshot 2024-07-17 150241

After:

Screenshot 2024-07-17 150259

Also on my side there seems to be a problem with text overlapping

Please can you check if that fixes the issue you saw @IAmMarcelJung?

I've also fixed some missing indentations in the docstrings.

Cheers,
Andrew.

@IAmMarcelJung
Copy link
Collaborator

Thanks a lot, now it also looks good on my side!

@A-Kibats A-Kibats force-pushed the Documenting branch 2 times, most recently from 89f4f38 to 76e5d48 Compare July 18, 2024 13:09
@KelvinChung2000
Copy link
Collaborator

@IAmMarcelJung If you are happy with the current state of the new Doc I will merge this.

Copy link
Collaborator

@IAmMarcelJung IAmMarcelJung left a comment

Choose a reason for hiding this comment

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

First of all thanks again for all the changes! After going through each of them I appreciate them even more! I think I misused the review feature a bit, since I also suggested changes for some parts that were already present before and were just copied due to the documentation changes. Sorry for that, but at some point I couldn't really stop... I can also not recommend it, since GitHub got painfully slow (it took 30 seconds to make a small suggestion in the end).

FABulous/FABulous.py Outdated Show resolved Hide resolved
FABulous/FABulous.py Outdated Show resolved Hide resolved
FABulous/FABulous.py Outdated Show resolved Hide resolved
FABulous/FABulous.py Outdated Show resolved Hide resolved
FABulous/FABulous.py Outdated Show resolved Hide resolved
FABulous/fabric_generator/fabric_gen.py Outdated Show resolved Hide resolved
FABulous/fabric_generator/fabric_gen.py Outdated Show resolved Hide resolved
FABulous/fabric_generator/fabric_gen.py Outdated Show resolved Hide resolved
FABulous/fabric_generator/fabric_gen.py Outdated Show resolved Hide resolved
FABulous/fabric_generator/fabric_gen.py Outdated Show resolved Hide resolved
@IAmMarcelJung
Copy link
Collaborator

@KelvinChung2000 For me everything works fine now. When I showed the new design to Dirk (since he's not active here and I try to keep him a bit up to date), he wasn't quite happy about it, so probably ask him too.

@KelvinChung2000
Copy link
Collaborator

Last time I talked to him, is because he dislikes dark themes. Maybe peek the light theme version to him and see what else he would like to change. Since this is only about the theme, once the correction is done, I will have this merged as well.

Applied the corrections done by @IAmMarcelJung. Big thanks to him!

Co-authored-by: Marcel Jung <[email protected]>
@KelvinChung2000 KelvinChung2000 merged commit a7a9290 into FPGA-Research:FABulous2.0-development Aug 2, 2024
2 checks passed
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